Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List Django as a dependency. Fix #96 #132

Merged
merged 1 commit into from
Mar 7, 2018
Merged

Conversation

atodorov
Copy link
Contributor

@atodorov atodorov commented Mar 5, 2018

also add a big warning statement about possible side effects and to deter people from reporting issues about Django version mismatches. Updates README formatting a bit as well.

@jakirkham this should fix #96.

@carlio I've read your reasoning about not listing this as a dependency and the side effects it may have. Let's see if the warning text I've added has any effect. If we start seeing consistent reports about Django version mismatches I'm all for dropping the Django dependency from setup.py and adjusting the warning so that folks are aware they have to install Django explicitly.

also add a big warning statement about possible side effects and
to deter people from reporting issues about Django version
mismatches. Updates README formatting a bit as well.
@atodorov atodorov requested a review from carlio March 5, 2018 21:38
@coveralls
Copy link

Pull Request Test Coverage Report for Build 293

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 84.826%

Files with Coverage Reduction New Missed Lines %
pylint_django/transforms/fields.py 1 98.11%
Totals Coverage Status
Change from base Build 291: 0.0%
Covered Lines: 464
Relevant Lines: 547

💛 - Coveralls

3 similar comments
@coveralls
Copy link

Pull Request Test Coverage Report for Build 293

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 84.826%

Files with Coverage Reduction New Missed Lines %
pylint_django/transforms/fields.py 1 98.11%
Totals Coverage Status
Change from base Build 291: 0.0%
Covered Lines: 464
Relevant Lines: 547

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 293

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 84.826%

Files with Coverage Reduction New Missed Lines %
pylint_django/transforms/fields.py 1 98.11%
Totals Coverage Status
Change from base Build 291: 0.0%
Covered Lines: 464
Relevant Lines: 547

💛 - Coveralls

@coveralls
Copy link

coveralls commented Mar 5, 2018

Pull Request Test Coverage Report for Build 293

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 84.826%

Files with Coverage Reduction New Missed Lines %
pylint_django/transforms/fields.py 1 98.11%
Totals Coverage Status
Change from base Build 291: 0.0%
Covered Lines: 464
Relevant Lines: 547

💛 - Coveralls

@atodorov atodorov merged commit 9b520e9 into master Mar 7, 2018
@atodorov atodorov deleted the make_django_requirement branch March 7, 2018 09:44
@JensTimmerman
Copy link

JensTimmerman commented Mar 8, 2018

I don't really like this change.
I use prospector for a bunch of python projects that are not at all django related.
Since I use prospector pylint-django get's installed automatically, this was not an issue, until now, because pylint-django now tries to install Django as well. (version 2.x, which doesn't even work on python 2.7)
So all my tests now try to install django, and fail.

Not sure if pylint-django is to blame, or prospector, but it seems at least the pylint-django developers are aweae that pylint-django is auto installed with prospector...

The readme changes warns me not to open issues about this, but my project doesn't involve django at all, so am I still required to fix my testing setup and install a django, even when nothing in my project needs it?

@jakirkham
Copy link
Contributor

This sounds like a poor separation of concerns, @JensTimmerman. Really prospector should soften the pylint-django requirement if it is not always needed. Have you brought this up with the prospector devs?

@JensTimmerman
Copy link

@carlio
Copy link
Member

carlio commented Mar 8, 2018

It's not a poor separation of concerns until recently - the point of prospector was to be a super easy "run one command" static analysis tool so it made sense to include pylint-django because automatically tuning checks for whatever framework you were using was the initial focus.

I realise now that the propagation of requirements was another reason I didn't want to include django as a requirement of pyling-django. By removing pylint-django as a default install for prospector, it undermines the whole point which was "install, point it at code, get results with as little effort as possible".

I still maintain that not including Django as a dependency of pylint-django is a good idea, people who don't set up their test environment properly need to deal with that and including as many warnings and clues as possible in pylint-django is the best idea - hence the DjangoNotInstalledChecker. Make it easy to diagnose but don't try to second guess every environment it will be installed into because that's too many permutations.

@carlio
Copy link
Member

carlio commented Mar 8, 2018

(Also I should point out that the "prospector devs" is me and just me, and until recently that was true of pylint-django too, so... I am the main one to blame for faults in both 🤕 )

@atodorov
Copy link
Contributor Author

atodorov commented Mar 8, 2018

@carlio as a pylint-django developer and user I'm not really interested in prospector and I'm more in favor with @jakirkham that prospector needs to relax the pylint-django dependency.

OTOH I recognize there's a valid use-case for prospector to pull in pylint-django automatically but not pull Django because, well you should already have Django.

IMO both the fact that prospector has a dependency on pylint-django and the fact that I and maybe others were not aware of this lead to the conflict. @JensTimmerman brings up a valid use-case too, I don't argue about it.

Can we all agree to revert the Django dependency from pylint-django and improve README where the fat warning will say that Django must be installed manually ? I can also add a GitHub issues template to warn users not to report bugs against that.

@atodorov
Copy link
Contributor Author

atodorov commented Mar 8, 2018

How about an optional dependency on Django?

pip install pylint-django[with-Django]

plus the above suggestion for README ?

@jakirkham
Copy link
Contributor

Sorry the argument doesn't seem very persuasive to me.

One cannot even import pylint-django without django being installed. So it doesn't make sense to have pylint-django without this being a hard requirement. One could probably investigate softening the django requirement to the point that pylint-django installs and is importable without it (perhaps with all or nearly all functionality disabled). Not sure this is worth the effort, but will happily let pylint-django developers make that decision.

My recommendation would be that prospector soften the pylint-django dependency, but add it as extra requirement. That way one could do something like pip install prospector[all] and get pylint-django and any other cool, optional dependencies along with it. However if users want to install prospector without pylint-django that would be an option as well. Would think this added flexibility would make prospector a more appealing tool to users not less, but could be wrong about that.

@atodorov
Copy link
Contributor Author

atodorov commented Mar 8, 2018

I think there's a bug in the existing code base and the django_installed checker does not work. IMO this is the source of @jakirkham's arguments and probably some confusion too.

Here's the output on a brand new virtualenv without Django (pylint-django 0.9.1):

$ pylint --load-plugins=pylint_django ~/Kiwi/tcms
Traceback (most recent call last):
  File "/home/senko/.virtualenvs/testme/bin/pylint", line 11, in <module>
    sys.exit(run_pylint())
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/pylint/__init__.py", line 16, in run_pylint
    Run(sys.argv[1:])
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/pylint/lint.py", line 1268, in __init__
    linter.load_plugin_modules(self._plugins)
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/pylint/lint.py", line 494, in load_plugin_modules
    module = modutils.load_module_from_name(modname)
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/astroid/modutils.py", line 190, in load_module_from_name
    return load_module_from_modpath(dotted_name.split('.'), path, use_sys)
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/astroid/modutils.py", line 233, in load_module_from_modpath
    module = imp.load_module(curname, mp_file, mp_filename, mp_desc)
  File "/home/senko/.virtualenvs/testme/lib64/python3.6/imp.py", line 245, in load_module
    return load_package(name, filename)
  File "/home/senko/.virtualenvs/testme/lib64/python3.6/imp.py", line 217, in load_package
    return _load(spec)
  File "<frozen importlib._bootstrap>", line 684, in _load
  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/pylint_django/__init__.py", line 7, in <module>
    from pylint_django import plugin
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/pylint_django/plugin.py", line 5, in <module>
    from pylint_django.augmentations import apply_augmentations
  File "/home/senko/.virtualenvs/testme/lib/python3.6/site-packages/pylint_django/augmentations/__init__.py", line 19, in <module>
    from django.views.generic.base import View, RedirectView, ContextMixin
ModuleNotFoundError: No module named 'django'

atodorov added a commit that referenced this pull request Mar 9, 2018
This reverts commit 9b520e9.

Apparently this is causing more problems than it solves, see:
#132
@atodorov atodorov mentioned this pull request Mar 9, 2018
atodorov added a commit that referenced this pull request Mar 12, 2018
This reverts commit 9b520e9.

Apparently this is causing more problems than it solves, see:
#132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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