This makes it easier to reuse pieces of the dagster-graphql schema, as well as smoothing the path for new contributors. Resolves https://github.com/dagster-io/dagster/issues/2270.
Details
Details
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Branch
- guillotine
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py | ||
|---|---|---|
| 5 | where there are collisions between the native/dagster type and the graphene type, we import the dagster type with the prefix Dagster | |
| python_modules/dagster-graphql/dagster_graphql/implementation/utils.py | ||
| 9 | rename | |
| python_modules/dagster-graphql/dagster_graphql/schema/__init__.py | ||
| 8–25 | Previously we were relying on the implicit registry functionality / metaclass magic provided by Dauphin to load all of the types in the schema; this moves us to be explicit, which may or may not be better, but more importantly allows pieces of the schema to be imported by other projects without having to pull in the entire registry | |
| python_modules/dagster-graphql/dagster_graphql/schema/asset_key.py | ||
| 2 | There's nothing final or definitive about this reorganization, but I have tried to break up the very large files in dagster-graphql into more manageable chunks | |
| python_modules/dagster-graphql/dagster_graphql/schema/backfill.py | ||
| 32 | retain Meta for unions | |
| python_modules/dagster-graphql/dagster_graphql/schema/config_types.py | ||
| 41 | drop this boilerplate | |
| 47 | wrap in lambda to avoid circular reference issue | |
| 100 | move all interfaces to use tuples rther than lists | |
| 215–248 | could also be __types__ or __all__ | |
| python_modules/dagster-graphql/dagster_graphql/schema/errors.py | ||
| 9–11 | this moved off dauphin | |
| 27 | python 3 super throughout | |
| 95 | more format strings | |
| python_modules/dagster-graphql/dagster_graphql/schema/inputs.py | ||
| 2 | moved input types together | |
| python_modules/dagster-graphql/dagster_graphql/schema/jobs.py | ||
| 27 | no need for this to go through dauphin wrapper without the registry | |
| python_modules/dagster-graphql/dagster_graphql/schema/logs/__init__.py | ||
| 2 | lazy to avoid any import issues and allow subsets of the schema to be imported | |
| python_modules/dagster-graphql/dagster_graphql/schema/runs.py | ||
| 21 | here's an example of a fully-qualified string reference | |
| python_modules/dagster-graphql/dagster_graphql/schema/solids.py | ||
| 87–88 | trying to be explicit | |
Comment Actions
I think this all makes sense to me.
We should probably merge this after the release today... (good luck with rebasing?) as we're probably relying more than usual on graphql test coverage to surface issues.
Comment Actions
Are we losing any greppability here? It was kind of nice that all the
graphql type classes had a common prefix, we could consider adding a
GraphQL prefix or something by convention? Or is that what python modules
are for :)
Comment Actions
Very much support getting rid of the registry. Thanks for doing this! Let's just try to mitigate risk as much as possible with a diff this large
Comment Actions
happy to reintroduce the prefix, it will entail a whole bunch of overhead in Meta classes but so be it -- does anyone have suggestions for the prefix? Gql?