Provide path_to_schema to schema definition state - #1316
Conversation
| @schema_artifacts_directory = schema_artifacts_directory | ||
| # Supports extensions that need to maintain files alongside the schema definition, rather | ||
| # than in the schema artifacts directory. | ||
| @path_to_schema = path_to_schema |
There was a problem hiding this comment.
It seems weird to store it on SchemaArtifactManager when it's not used by SchemaArtifactManager. And instance variables are in general a more brittle extension mechanism because if of how Ruby treats ivars: they spring into existence when first referenced, so if you mispell the name of the ivar, Ruby will just make a new one.
An alternate suggestion:
- Store
path_to_schemaonSchemaDefinitition::State - Change
SchemaArtifactManagerto acceptschema_def_api(which has access tostate) rather thanschema_definition_results--it can callschema_def_api.resultsto get the results, and that way an extension has access toschema_def_api.state.path_to_schema.
Separately, it's worth considering if it's best to have SchemaArtifactManager generate the proto numbers file. If we're saying it's not a schema artifact, it seems odd to have the SchemaArtifactManager dump it!
That said:
- Apart from the slight naming mismatch, I don't see a reason to not have
SchemaArtifactManagerdump it. - I'd only consider having it generated somewhere else if it was just as simple. If it's much more complex to dump it from somewhere else then I'd rather do it in a
SchemaArtifactManagerextension in spite of the name mis-match. - One concrete thing the
SchemaArtifactManagerdoes is checks all the artifacts (when you runbe rake schema_artifacts:check). It's not clear to me if that's meaningful and useful for the proto numbers file or not. (I'm guessing it is, becaues that's probably how you'd ensure every field and enum value is in the file.)
Thoughts?
Schema definition extensions may need to maintain inputs alongside the schema file. Carry the configured path on State so extensions can access it without expanding SchemaArtifactManager dependencies.
20d16fd to
41f7655
Compare
myronmarston
left a comment
There was a problem hiding this comment.
LGTM apart from a suggestion below and one other thing: the PR description is no longer accurate. Worth updating before you merge?
| @api = SchemaDefinition::API.new( | ||
| SchemaArtifacts::RuntimeMetadata::SchemaElementNames.new(form: :camelCase, overrides: {}), | ||
| true, | ||
| path_to_schema: "#{@tmp_dir}/schema.rb", |
There was a problem hiding this comment.
Does "#{@tmp_dir}/schema.rb" even exist? I'd prefer not to lie and claim it does if it doesn't exist. And it looks like the path_to_schema: arg is optional...should we just omit it here or do we need it for some reason I'm not thinking of?
path_to_schema to SchemaArtifactManagerpath_to_schema to schema definition state
Today, every schema artifact is a pure function of the schema definition: the schema artifacts directory can safely be deleted and regenerated at any time. In #1304 (comment), @myronmarston pointed out that
proto_field_numbers.yamldoesn't fit that model--it's an input toschema.protogeneration, not a pure output--and suggested treating it as part of the schema definition, stored as a sibling ofpath_to_schema, rather than as a schema artifact.For an extension to maintain a file there, schema definition state needs to know where the schema definition lives.
RakeTasksalready knows (it's how the schema gets loaded) but never passed it into the API. This PR storespath_to_schemaonSchemaDefinition::State, allowing extensions to access it without expandingSchemaArtifactManager's dependencies.Nothing in core uses the value yet, so there are no behavior changes to the core artifacts. #1304 builds on this to relocate
proto_field_numbers.yaml.Update — 2026-07-29
After review, the path is carried by schema definition state rather than an extension-only
SchemaArtifactManagerinstance variable. The manager continues to accept the narrower schema-definition results object.