make django a requirement, or somehow let users know that it has to be installed

[jo-tham]

Hi,

thanks for an awesome plugin; it was a relief to find when I introduced pylint to an existing django project and was swarmed with errors from django class Meta and so forth.

My team had an issue with getting the plugin to work in some instances.

Specifically; using a virtualenv/global installation of pylint and plylint django which did not include django. It may seems silly, but I think it's a valid usage pattern.

The django typical lint errors kept being reported, and we were pretty confused as to why. Of course, django had to be installed on path.

I didn't see anywhere in the readme that django had to be installed. It's a minor nuisance to solve, but it'd be nice to help the next person by either:

• letting them know in the readme
• raising an error when pylint_django is loaded if django is not installed
• adding django to requirements in setup.py

I'm in favour of the last option. Glad to create a PR for the option that the maintainer(s) prefer, if any.

Thanks

[carlio]

@jo-tham There is already a checker to do that here: https://github.com/landscapeio/pylint-django/blob/master/pylint_django/checkers/django_installed.py

So if you did not get the warning then that is probably broken! There have been many version of pylint since it was originally written so perhaps the API changed somewhere. I'll look into it :-)

[atodorov]

With the latest version I get a ModuleNotFoundError: No module named 'django' exception if Django is not found in the path. I'm not sure the existing checker will work b/c Django is imported in other modules before the checker is registered.

I am in favor of closing this issue and removing the django_installed.py checker. @carlio @colinhowe what do you think ?

[carlio]

The checker was at least originally loaded before other things - it was there to avoid needing to specify Django as a requirement to avoid polluting peoples' environments as who knows what version or how they installed it. That checker is there really just as a hint for when things go wrong. It can go away for sure, it's not really important except to help quickly diagnose why pylint-django isn't working, it's not a code quality issue.

@atodorov It'd be nice to keep that warning (since some CI environments might not have a full dependency tree) but the checker itself can go.

[carlio]

Also https://github.com/PyCQA/pylint-django/blob/master/pylint_django/compat.py#L23 would need to be changed a bit!

[atodorov]

@carlio I can always try import django and exit gracefully before anything else and we'll not have to update the compat layer. Will give it a try later.

[carlio]

Also Here: https://github.com/PyCQA/pylint-django/blob/master/pylint_django/augmentations/__init__.py#L26 (for additional reasons why this was in place)

[jakirkham]

Ran into an issue caused by this recently. Any reason not to include django as a hard requirement?

[carlio]

@jakirkham to be honest it was because if you're testing the validity of your django code in an environment without django, you have bigger problems :-) I don't quite understand why anyone would need the ability to have a test library define what needs to be installed? I don't mind this requirement being dropped, just seems odd to me, especially given pylint's inference, that you wouldn't already have the dependencies involved in the test environment. Otherwise pylint will miss lots of stuff too, it can't follow the AST if nodes aren't there.

[jakirkham]

Think there is some confusion. Not encouraging django being dropped. Am encouraging it being added as an explicit dependency (since it isn't currently). In which case, sounds like we are in agreement? :)

The reason for the request is we maintain a package for this in conda-forge and we try to mirror upstream's requirements. However as django is not required here, we didn't include it in our package, which obviously breaks. So we have added the dependency even though it deviates with what we have here. Would be best if we could add the dependency here too.

[carlio]

@jakirkham Sorry I meant "I don't mind this requirement to not depend on django being dropped" - bad phrasing on my part.

I think as long as the dependency in setup.py has no version restrictions then I don't see any big issue, though if for whatever reasons people don't have django already in their testenv they'll get latest Django which might not be what they're coding towards and then they'll get incorrect errors and then they'll complain and post issues here.

That was also a motivation, the fact that by specifying a version you might conflict with the user's requirements and by not specifying a version you only get latest which might also conflict with the user's requirements. That's why there's a "No Django" checker, to get them to make sure they have the right version installed for them.

[jakirkham]

No worries. Thanks for clarifying.

Personally would bias towards adding the dependency and informing users to better constrain their environments if they raise such issues. Also would think that would be a pretty defensible position. Those that are already doing this correctly in development would be unaffected.

[carlio]

Agreed - that's what the DjangoNotInstalledChecker was for really. It came about from getting lots of bogus bug reports due to improperly configured test environments, not sure what the best idea is since pylint-django deals with a range of versions but adding if DjangoVersion==x.y to every check would make it a mess. Other than a quick-reply for github issues :-)

[jakirkham]

Interesting. Maybe promoting better tooling for managing testing environments, virtualenv and pipenv come to mind. For those with more complex requirements, conda. Could also look into GitHub issue and contributing templates to catch this stuff before the issue is filed.