Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swift: add more doc strings to generated things #14715

Merged
merged 5 commits into from
Nov 10, 2023
Merged

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Nov 7, 2023

Also, all files under generated and their classes are now marked INTERNAL regardless of the ql.internal pragma, as they should never be used directly (a user should not write Generated::Bla).

@redsun82 redsun82 added the no-change-note-required This PR does not need a change note label Nov 7, 2023
@redsun82 redsun82 requested a review from a team as a code owner November 7, 2023 15:30
@github-actions github-actions bot added the Swift label Nov 7, 2023
@redsun82 redsun82 requested a review from geoffw0 November 7, 2023 15:32
geoffw0
geoffw0 previously approved these changes Nov 7, 2023
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (I checked the generation code and a sample of the generated comments). These are a bit repetitive, but I think will really help a reader who hasn't figured out what each type of file is for already.

swift/ql/lib/codeql/swift/elements.qll Show resolved Hide resolved
AlexDenisov
AlexDenisov previously approved these changes Nov 8, 2023
@@ -1,4 +1,10 @@
// generated by {{generator}}, remove this comment if you wish to edit this file
/**
* This module provides a hand-modifiable wrapper around the generated class `{{name}}`.
{{#ql_internal}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure ql_internal is part of the Stub?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! And it made me think, that all the generated classes should actually be marked INTERNAL, as they should never be used directly, so I changed that as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But methods from generated classes are used all the time and thus should not be marked as internal, right? For example Callable.getName() is from Generated::Callable.

Copy link
Contributor Author

@redsun82 redsun82 Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly. Unless ql.internal is used on a specific property (for example kind: int | ql.internal), getters are not marked as internal.

@redsun82 redsun82 dismissed stale reviews from AlexDenisov and geoffw0 via b7543f5 November 8, 2023 08:54
@redsun82 redsun82 requested a review from geoffw0 November 8, 2023 09:05
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good now. 👍

I was expecting a minor merge conflict with #14261 , but I can't see any evidence this happened.

@redsun82
Copy link
Contributor Author

Looks all good now. 👍

I was expecting a minor merge conflict with #14261 , but I can't see any evidence this happened.

The conflict was there on .generated.list, but it only required a bazel run //swift/codegen -- --force.

@redsun82 redsun82 merged commit b611e7c into main Nov 10, 2023
17 checks passed
@redsun82 redsun82 deleted the redsun82/gen-file-docs branch November 10, 2023 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants