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

Hide OneOfSchema Attributes #139

Merged
merged 19 commits into from
Sep 5, 2024
Merged

Hide OneOfSchema Attributes #139

merged 19 commits into from
Sep 5, 2024

Conversation

mfleader
Copy link
Member

@mfleader mfleader commented Sep 4, 2024

Changes introduced with this PR

All attributes on the *Schema classes, like OneOfStringSchema or OneOfIntSchema, in schema.py must be public because the schema builder portion of schema.py uses metadata analysis of that class's attributes to determine if an instance of the Arcaflow type has been written as specified by the Arcaflow type system (implemented by the *Schema classes and a few other parts of this code base). As a language feature of Python, classes do not have private/hidden attributes. If a private attribute must exist on a *Schema class, then one solution is to encapsulate the required behavior or state on a part of the class that is not used as a part of the metadata analysis. In this pull request I have encapsulated the state we would like to be hidden within method calls on the relevant classes.

Testing for this behavior is not simple, and will have to be introduced in a follow-up pull request.


By contributing to this repository, I agree to the contribution guidelines.

@mfleader mfleader self-assigned this Sep 4, 2024
@mfleader mfleader added the bug Something isn't working label Sep 4, 2024
@mfleader mfleader marked this pull request as ready for review September 4, 2024 22:25
src/arcaflow_plugin_sdk/schema.py Outdated Show resolved Hide resolved
src/arcaflow_plugin_sdk/schema.py Outdated Show resolved Hide resolved
Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

I like Dave's suggestion (assuming it addresses the problem).

src/arcaflow_plugin_sdk/schema.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Looks good. I haven't tested it with a plugin.

@mfleader
Copy link
Member Author

mfleader commented Sep 5, 2024

For comparison, attached are the manually generated files of the Stress-NG plugin schema using the sdk's main branch, and this pull request's branch, oneof-attrs-privatize.

To reproduce:

❯ mkdir testdir
❯ cd testdir
❯ git clone https://github.com/arcalot/arcaflow-plugin-sdk-python.git
❯ git clone https://github.com/arcalot/arcaflow-plugin-stressng.git
❯ cd arcaflow-plugin-sdk-python/src
❯ cp -r ../../arcaflow-plugin-stressng/arcaflow_plugin_stressng .
❯ python arcaflow_plugin_stressng/stressng_plugin.py --schema > stressng_schema-main.txt
❯ git switch oneof-attrs-privatize
❯ python arcaflow_plugin_stressng/stressng_plugin.py --schema > stressng_schema-oneof-attrs-privatize.txt
❯ diff stressng_schema-main.txt stressng_schema-oneof-attrs-privatize.txt
760,761d759
<                   discriminator_type: string
<                   oneof_type: _discriminated_string_
2367,2368d2364
<                       discriminator_type: string
<                       oneof_type: _discriminated_string_

GitHub doesn't want me to upload a yaml file, so they are txt files.
stressng_schema-main.txt
stressng_schema-oneof-attrs-privatize.txt

@mfleader mfleader requested a review from webbnh September 5, 2024 19:19
@mfleader mfleader requested a review from dbutenhof September 5, 2024 19:19
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Oh, cool. 😁

@mfleader mfleader merged commit 6dcecbd into main Sep 5, 2024
4 checks passed
@mfleader mfleader deleted the oneof-attrs-privatize branch September 5, 2024 21:49
webbnh

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants