-
Notifications
You must be signed in to change notification settings - Fork 9
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
chore(sca): add comments to manifest kinds #305
Conversation
Backwards compatibility summary:
|
cc69438
to
8669f34
Compare
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.
lgtm!
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.
nice
(* TODO: merge with Manifest_kind.ml *) | ||
type manifest_kind <python decorator="dataclass(frozen=True)"> = [ | ||
| RequirementsIn (* Manifest file for pip *) | ||
type manifest_kind |
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.
@mjambon what do we need to add to generate proper Variant like RequirementsIn instead of the `RequirementsIn in the generated OCaml code?
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, it looks like that might even be supported today with <ocaml repr="classic">
: https://atd.readthedocs.io/en/latest/atdgen.html#sum-types
Do you think I should switch this one over?
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.
I prefer yes. They are simpler to handle.
In order to consolidate the
manifest_kind
type in interfaces withManifest_kind.ml
, this PR:transfers the comments from the ML file to interfaces
adds
eq
,show
, andyojson
derivations to the ATD definitionI ran
make setup && make
to update the generated code after editing a.atd
file (TODO: have a CI check)I made sure we're still backward compatible with old versions of the CLI.
For example, the Semgrep backend need to still be able to consume data generated
by Semgrep 1.17.0.
See https://atd.readthedocs.io/en/latest/atdgen-tutorial.html#smooth-protocol-upgrades