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

Add support for copy directives #35

Merged
merged 3 commits into from
Nov 14, 2024
Merged

Conversation

Silvanoc
Copy link
Contributor

@Silvanoc Silvanoc commented Aug 1, 2024

As of now CopyDirective objects were supported by the specification, but the implementation to handle them was missing.
This PR adds support for them.

@Silvanoc Silvanoc force-pushed the add-support-for-copy-directives branch 3 times, most recently from 8f28d95 to 871c49b Compare August 1, 2024 11:26
@Silvanoc
Copy link
Contributor Author

Silvanoc commented Aug 1, 2024

No support yet for following CopyDirective properties:

  • exclude_all
  • include
  • add

Still working on them, will be included before marking the PR ready.

@Silvanoc Silvanoc force-pushed the add-support-for-copy-directives branch 2 times, most recently from 6dcc9c4 to d69f1ad Compare August 1, 2024 16:39
@Silvanoc
Copy link
Contributor Author

Silvanoc commented Aug 1, 2024

@sierra-moxon could you give me some guidance on what should happen if exclude_all and copy_all are simultaneously specified in a CopyDirective?

Is it a conflict that should raise an error?
Does exclude_all win always and can only be modified with include?
Does copy_all win always and can only be modified with exclude?

Additionally, what is the slot add for in a CopyDirective?

I am not sure if these questions are worth a new issue...

@Silvanoc Silvanoc force-pushed the add-support-for-copy-directives branch from 0392afb to 7b54b84 Compare August 1, 2024 17:12
@Silvanoc Silvanoc force-pushed the add-support-for-copy-directives branch from 7b54b84 to 294ea91 Compare August 2, 2024 08:42
@cmungall
Copy link
Member

cmungall commented Aug 3, 2024

I would say raise an error

@cmungall
Copy link
Member

cmungall commented Aug 7, 2024

The conflict arose from different versions of pydanticgen, could be resolved manually but probably easiest for you to regen on your branch?

@Silvanoc
Copy link
Contributor Author

I would say raise an error

According 0164002, no error would be needed. exclude_all would simply override and therefore "disable" copy_all.

It would help usability, but not further than that.

Most tools that I know supporting similar filters simply specify priority rules. It is then up to the user to be smart enough on the applied filters and the expected result.

@cmungall
Copy link
Member

@Silvanoc apologies for the delay. I agree with your proposed interpretation. lmk how you want to proceed with the PR - happy to do this incrementally if we want to merge the current work once the conflict is resolved

As of now copy_directives can not be specified at
TransformationSpecification level to declare that all elements of a
schema should be recursively copied.
This commit adds this missing support.

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
As of now CopyDirective objects were supported by the specification, but
the implementation to handle them was missing.
This commit adds support for them.

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
@Silvanoc Silvanoc force-pushed the add-support-for-copy-directives branch from 294ea91 to 91374bb Compare August 26, 2024 05:50
Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
@Silvanoc Silvanoc force-pushed the add-support-for-copy-directives branch from 91374bb to 8ed00a5 Compare August 26, 2024 07:00
@cmungall cmungall marked this pull request as ready for review November 13, 2024 23:21
@sierra-moxon sierra-moxon merged commit 0904bb2 into main Nov 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants