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

ENH: Produce inplace volumes result schema #957

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

mferrera
Copy link
Collaborator

@mferrera mferrera commented Jan 8, 2025

Resolves #895

This PR gets us 80% of the way to versioned schemas as well

Comment on lines +56 to +57
class InplaceVolumesGenerateJsonSchema(GenerateJsonSchema):
"""Implements a schema generator so that some additional fields may be added."""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In a follow-up PR I think I want to see if I can apply a generator schema generator from SchemaBase to use as a default

@mferrera mferrera force-pushed the dump-product-schema branch 2 times, most recently from 4b0d4b3 to 5df7391 Compare January 8, 2025 06:16
@@ -24,7 +24,7 @@ jobs:
- name: Check schema
run: |
./tools/update_schema
./tools/update-schema
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

POSIX convention is usually hyphens for executable names

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, when I read about (asking chatgpt), it says that "Underscores are more common in internal or development scripts where they are not directly exposed to users (e.g., build_script)." Hyphen is OK with me

Copy link
Collaborator Author

@mferrera mferrera Jan 9, 2025

Choose a reason for hiding this comment

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

I think this is a case where ChatGPT is happy to oblige. I get:

For scripts that are invoked as executables, hyphens (-) are generally more common than underscores (_) between words. This convention aligns with the broader Unix/Linux philosophy and naming practices, where hyphen-separated names are easier to type and visually distinct. For executable scripts, prefer hyphens (-) unless there's a compelling reason to use underscores (_).

Comment on lines +26 to +29
SCHEMAS = [
FmuResultsSchema,
InplaceVolumesSchema,
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here and in the tests we are collecting a list of all (two) the schemas to iterate over them. There should probably be a place in the code to collect them, but I think waiting for the next one to see where that should be might land it in a better spot (maybe?)

Comment on lines +47 to +51
parser.add_argument(
"--force",
action="store_true",
help="Force the script to overwrite the current schema with the new schema.",
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Until we roll out schema versioning completely we'll need to --force overwrite schema changes.

Also TODO: is add a --release flag that allows overwriting the $id to the prod url, when proposing to staging

@mferrera mferrera self-assigned this Jan 8, 2025
@mferrera mferrera marked this pull request as ready for review January 8, 2025 06:24
@mferrera mferrera requested review from tnatt and joargr January 8, 2025 06:24
Copy link
Collaborator

@jcrivenaes jcrivenaes left a comment

Choose a reason for hiding this comment

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

LGTM

@mferrera mferrera force-pushed the dump-product-schema branch from 5df7391 to f8ff804 Compare January 9, 2025 11:06
@mferrera mferrera merged commit aaa4035 into equinor:main Jan 9, 2025
16 checks passed
@mferrera mferrera deleted the dump-product-schema branch January 9, 2025 11:09
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.

Define the end-point over which we serve data product schemas
2 participants