Repository navigation
Conversation
NelleV
changed the title
NelleV
changed the title
NelleV
changed the title
This patch also separates the tests and installation script from the main travis.yml file. closes #7358
|
@tacaswell Can we get this in? The current system is very fragile, and this makes it much more robust and consistent… |
| ${PYTHON} tests.py | ||
|
|
||
| test-coverage: | ||
| python tests.py |
There was a problem hiding this comment.
should this be ${PYTHON} as well?
|
|
||
| if args.no_pep8: | ||
| default_test_modules.remove('matplotlib.tests.test_coding_standards') | ||
| if '--no-pep8' in sys.argv: |
There was a problem hiding this comment.
This rebase went badly on top of the argparse work.
|
I do not understand what you mean by the current system is fragile. |
|
@tacaswell There are a bunch of files that can't be accessed, and comparing the number of failures before/after is a very incomplete solution (you can easily add a pep8 uncompliant line, and remove one - the tests will be perfectly happy with that.). But, as @lesteve mentioned on the related ticket, running flake8 on the diff is not perfect: you can miss elements such as imports not being used, or blank lines missing, as they are related to multiple line changes and not single line changes (I am not sure I am being very clear here). What triggered me doing this PR quickly is the discussions surrounding #7398 (where the pep8 related failures were on very old examples that have not be maintained recently). I think we need to lower the bar for contributions, in particular on elements such as examples, and the discussions led towards suggestion like pep8-ifying all the examples that were previously not checked (which is a tremendous amount of work). It delayed merging a PR that was a huge improvements in terms of maintainability, and underlined that those tests are currently fragile and poorly understood by even the core team. |
|
I think you might have misunderstood how the current system works. It does not run on the diff; it runs on all files in a few configured paths. Files either pass or do not; if they don't pass and they're excluded, then that failure is ignored. In #7398, the original location of those files was not one of those configured paths, so those files were never checked, but were moved into a path that was checked and thus started failing.
No-one made that suggestion at all. The suggestion from the beginning was always to exclude those files; I think you are mischaracterizing both my and @Kojoley's statements. |
|
@QuLogic I did not misunderstand the way it is done right now. The goal of this PR is exactly to alleviate the --diff function of pep8/flake8 to run on the diff of python files and not whole python. |
|
Is the perceived gain that this avoids the need to keep lists of files and paths? |
|
This is one benefit of this PR. |
|
Other benefit is of course maintenance. Tests need to be maintained, and we are removing elements that we need to maintain to a feature someone else is maintaining. |
|
OK, I think I misunderstood what you were saying initially. However, I don't think this will lower the bar for contributors; in fact, I think it will increase work. If pep8 violations are added to clean files, then both the existing and this PR will complain about it. If pep8 violations are added to excluded files, the existing code will not complain, but this PR will. This is great for maintenance later when we want to add compliance to every file, but the net effect is more work for new contributions. Really, the best way to lower the bar for new contributors is to make sure everything is pep8-compliant already, but that's a much bigger thing to do. |
|
@QuLogic That's also a very bad idea for many many reason. |
|
It's potentially more work if they're not writing pep8-compliant code, but checking the diff actually ensures that new code is compliant (aforementioned corner cases notwithstanding). IMO, this is something we very much should be doing, not punting it until some future date when we can eliminate the blacklist. |
|
Just to be clear, I think what we should avoid is trying to pep8ify everything at once (some may find the irony in this), as this is going to conflict with all the open PR we have. (Thought I have rarely seen people actively work on year-old PRs on matplotlib). |
|
I didn't say we should do that now, nor am I against this PR itself. PEP8'ing everything is just the only guaranteed way to lower the bar for new contributors (as far as style is concerned). |
|
I'm closing this PR. |
Just out of interest, why? Lack of consensus, lack of energy/time to change it, other reasons? |
|
@lesteve I've got too many PRs opened and this one has not received any attention since 20 days. |
|
The code is still here, so it is easy to reopen if any interest is expressed. |
QuLogic
changed the title
This patches removes the pep8 testing as it is done right now to make it more robust.
It runs flake8 on the diff instead and is external to our test suite.
This patch also separates the tests script from the main travis.yml file
TODO