Port Django migrations inference away from PythonDependencyVisitorRequest
#19008
Conversation
I might do this, just to start the cleanup 🤷♂️ |
| Digest, CreateDigest([FileContent("apps.json", django_apps.to_json())]) | ||
| ) -> InferredDependencies: | ||
| source_field = request.field_set.source | ||
| # NB: This doesn't consider https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-MIGRATION_MODULES |
There was a problem hiding this comment.
This comment make me wonder if it wouldn't make sense to implement this as a hybrid Django-Pants plugin, to leverage the settings and Django-isms you get from a Django plugin, but run in the context of a Pants managed setting to avoid re-implementing/re-discovering how Django works under the covers.
There was a problem hiding this comment.
So far we've avoided Pants-specific plugins to frameworks.
(That's my whole comment. Really this PR is just to support ripping out the old stuff)
There was a problem hiding this comment.
Really this PR is just to support ripping out the old stuff
yea, just got me thinking..
So far we've avoided Pants-specific plugins to frameworks.
Actively avoided? (I've missed that memo 😅 ) ((if so, why? OK, I can come up with a few cons to doing it that way... but also a few pros))
There was a problem hiding this comment.
I think it's fine to not worry about it. It's very rare to not call the migrations package migrations, and you always have the workaround of manual deps. If someone really needs this, they can add a Pants option to support that.
| Digest, CreateDigest([FileContent("apps.json", django_apps.to_json())]) | ||
| ) -> InferredDependencies: | ||
| source_field = request.field_set.source | ||
| # NB: This doesn't consider https://docs.djangoproject.com/en/4.2/ref/settings/#std-setting-MIGRATION_MODULES |
There was a problem hiding this comment.
I think it's fine to not worry about it. It's very rare to not call the migrations package migrations, and you always have the workaround of manual deps. If someone really needs this, they can add a Pants option to support that.
| ], | ||
| env_inherit={"PATH", "PYENV_ROOT", "HOME"}, | ||
| ) | ||
| def do_test_migration_dependencies(rule_runner: RuleRunner, constraints: str) -> None: |
There was a problem hiding this comment.
For future reference, I would have preferred as few changes to the test as possible, as it is the insurance policy that the underlying code change is good, and so keeping it stable is strong reassurance that that test hasn't been cooked to match, so to speak.
There was a problem hiding this comment.
+1. I did look at that and tried to see what the test changes where coming from.. but gave up ;)
There was a problem hiding this comment.
Sorry, this round-tripped through some code that assumed that we'd do Py3 only
There was a problem hiding this comment.
The core of the test has remained the exact same, however. The rest is the necessary tweaks and a little cleaning
This is the only in-repo plugin using
PythonDependencyVisitorRequest, meaning when the rust parser is the new only implementationPythonDependencyVisitorRequestand plumbing can be removed (sooner if we refactor)Implementation:
migrationsdirectory