-
Notifications
You must be signed in to change notification settings - Fork 12
chore(dep-resolution): add option to specify dependency configurations to include #421
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
base: main
Are you sure you want to change the base?
Conversation
8f7915d to
7107cd5
Compare
|
Backwards compatibility summary: |
mjambon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you'll get merge conflicts since the comments are now <doc text="..."> annotations.
| allow_local_builds: bool; | ||
| (* the configurations (e.g. gradle configurations) to resolve. If not specified, | ||
| * all configurations will be resolved. Supported only for Gradle currently. *) | ||
| include_dependency_configurations: string list option; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be:
?include_dependency_configurations: string list option;
because it makes the field optional (otherwise it's a weird JSON representation and probably a weird python representation too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok, the Python interface isn't bad. It's just the JSON representation for the option type that's unidiomatic if you don't pair option with a question mark on the field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if you want to default to the empty list rather than a None, it is achieved with
~include_dependency_configurations: string list;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, doing so will make the Python argument optional, right? I wanted to have the argument be required but nullable. But it's not important, I can change it
Adds a new parameter to the ResolveDependencies RPC that allows specifying a
list of configurations to include in the resolution. We could alternately do
this by RPC, but it seemed worth it to do it as a CLI flag so we can potentially
use it in the future.
make setup && maketo update the generated code after editing a.atdfile (TODO: have a CI check)For example, the Semgrep backend need to still be able to consume data
generated by Semgrep 1.50.0.
See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades
Note that the types related to the semgrep-core JSON output or the
semgrep-core RPC do not need to be backward compatible!
semgrep-proprietaryare approved and ready to merge once this PR is merged