RFC: new checker: ForeignKey models referenced by string #243
Comments
|
I use string constants by default. They are much better and cleaner way to specify models without circular import issues. I suggest reporting strings that do not match this pattern |
|
I think this makes sense, as long as the checker uses a new error code then people can filter that out by ignoring it in their pylint config. I certainly think that it's worth suggesting that imports are used over strings where possible so this is a 👍 from me. (I also personally think that using strings to avoid circular imports can point to a larger problem of project architecture but that is outside the scope of pylint-django!) |
|
@imomaliev I am with @carlio on this one. However a comment here:
The problem is not the string pattern but how the application is structured. For example in #241 the FK reference follows the 'app.Model' pattern, it is In my own application we've got |
|
@atodorov The actual import can be resoled by reusing django's internal resolution methods. I think there should be a default way for most users. I am not against on removing this complexity from this plugin. Maybe for my case there should be a separate plugin that will do resolution proprely |
|
@atodorov At least for backward compat we should move current string resolution move to separate module, so the people who are using this functionality in current state won't lose it |
|
In my current use case, I'm using django-oscar and I don't control how fields are defined in this project. It would be really neat if this plugin could use An alternative to keep this functionality but not increase the burden on the team could be to allow people to define aliases. |
|
"Lazy relationships" is a django documented feature, so this should be supported. It is true that its usage may be a symptom of a circular import, but there are many cases in which it can be used without any problem. |
The encouragement to define FK relations by concrete class rather than by the string reference pattern provided in the Django documentation, as an option to address projects shaped in the manner detailed below, seems more appropriate for a blog post on the subject of best-practices and desirable project design, rather than as an included advisory in a linting plugin. The former is more likely to be able to educate and inform the entities responsible for design decisions; the latter is more likely to be viewed as an annoyance to be suppressed. I agree that the use of concrete classes is preferable to the use of string references, but it is sometimes simply impossible to avoid the use of string references without violating a project's specifications and requirements. As an example, consider a project that includes the requirement of three tables, Alpha, Beta, and Gamma. Alpha is specified as having FK relations to both Beta and Gamma, Beta is specified to have a relation to Gamma that is independent of either's reverse-relation to records in Alpha, and Gamma is specified as having a OneToOne relation to Alpha that is independent of its reverse-relation to Alpha. These specifications are fixed, and not subject to change. Gamma is a table of purchase orders, Alpha is a table of production action-items, and Beta is a table tracking RMA requests. Purchase orders in this system all have an initial production action-item record in Alpha that is referenced by the OneToOne column in Gamma. RMA requests in Beta also tie back to the initial purchase order record in Gamma. Satisfaction of the requests tracked in Beta or Gamma may require additional production action-item records in Alpha; if such additional items are necessary, direct links back to the relevant action-generating record are specified as necessary. I'm not aware of a way to define these models in a way that permits all of the FK-relations to employ concrete classes in the definitions of their Foreign Key fields, as the spec for the project defines and demands a circular relationship. The best one could do, I think, is to define the models in a single file, ordered Gamma, Beta, Alpha, and have the OneToOneField on Gamma as the only string-defined relation, but this is a fairly simple example, and ignores the external context of other possible relations, app organization, and the larger scope of the project, all of which might preclude this "best possible" implementation. The frequency with which this has arisen in my own experience, coupled with the frequency with which I've heard colleagues and friends describe the same kind of thing, suggests to me that a majority of developers will react to the proposed addition by disabling it. |
|
I wanted to use pylint-django for a project but ran into this issue. I like @imomaliev 's idea of using Django to resolve imports. Here's a quick change I came up with: alejandro-angulo@2c4ad3e . @atodorov What do you think of this approach? |
IDK how the internals you've used work but looks good at least on the surface. Open a PR with it so we can add tests and examine it further. |
|
@alejandro-angulo is there any way to get this fix to be compatible with Django 2.x.x as well? |
|
@ajhodges I looked at the Django docs and I don't think the version check is necessary so this fix should apply to 2.x.x as well (see PR mentioned below). @atodorov Finally got around to making that PR https://github.com/PyCQA/pylint-django/pull/272/files . I wasn't sure how to handle a |
…#243 this commit tries to load & configure Django and use its internal machinery to resolve apps and models in case they are referenced as a string. If this fails it falls back to appending ".models" to the first part of the name and looking for a module with that name to augment.
|
Now that @alejandro-angulo has implemented the resolution code for models referenced as strings I don't think we need a separate checker for that. If you still want to see a separate checker for FK fields referenced as strings (may indicate problems with layout/dependencies in the app) please upvote this comment. If you think this isn't necessary please downvote this comment. |
|
I do still feel like it represents a code smell that should be highlighted, but not as a high severity error just as a warning. But perhaps I'm just pernickety. |
…he `open()` pre-configuration. This allows the django settings module to be set from a command-line option or .pylintrc option as well as an environment variable (refs #286). This commit also uses default Django settings if none are specified, raising an error to the user to inform them that it is better to set it explicitly, but avoiding the need to raise a Fatal or RuntimeError if Django is not configured (refs #277 and #243)
Any thoughts on the @Doskious arguments and his example? Are there any discouragement of string constants in Django docs or encouragement of not using it? If there are none, why? |
ref pylint-dev#243 # Conflicts: # pylint_django/transforms/foreignkey.py
Given what we have in our Known issues section (in README) I propose that by default pylint-django starts reporting on FK and any other related field types which reference the related model via string constant not its class.
pylint-django probes for some relatively simple and rather common locations when trying to search for related models, documented in #242. However we can't and will not try to probe other possible locations because this will turn into maintenance hell (patching paths and expectations) and is not really the scope of this tool IMO. This will (hopefully) minimize the number of bug reports and the general misunderstanding about these types of issues.
I am opening this issue to serve as a place for comments, 👍 and 👎 reactions to see what other people think.
The text was updated successfully, but these errors were encountered: