Provide path_to_schema to SchemaArtifactManager - #1316
Open
jwils wants to merge 1 commit into
Open
Conversation
The schema artifacts directory is safe to delete and regenerate: every artifact is a pure function of the schema definition. Some extensions need to maintain a file that doesn't fit that model--one that is an *input* to artifact generation and therefore belongs alongside the schema definition itself rather than in the artifacts directory. The manager previously had no way to know where the schema definition lives, so give it `path_to_schema`, which `RakeTasks` already knows.
jwils
requested review from
BrianSigafoos-SQ,
bsorbo,
ellisandrews-toast,
jwondrusch,
marcdaniels-toast,
myronmarston and
rossroberts-toast
as code owners
July 25, 2026 15:13
myronmarston
requested changes
Jul 25, 2026
| @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 |
Collaborator
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?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, the
SchemaArtifactManagerneeds to know where the schema definition lives.RakeTasksalready knows (it's how the schema gets loaded) but never passed it along. This PR plumbspath_to_schemathroughFactory#new_schema_artifact_managerinto the manager so that extensions which maintain schema-definition-adjacent files can use it.Nothing in core uses the value yet, so there are no behavior changes to the core artifacts. #1304 will build on this to relocate
proto_field_numbers.yaml.