Initial attempt at finding unexecuted files #39
Conversation
|
Excuse my briefness, pull requests were made from my phone, as I was leaving the train I was in. |
PamelaM
left a comment
PamelaM
left a comment
There was a problem hiding this comment.
This seems a reasonable first pass at the proposed coverage.py plugin api.
I think the performance requirements of this code are pretty minimal, so future maintainability (i.e. simplicity) suggests dropping the regex
| res = [] | ||
| for i, (dirpath, dirnames, filenames) in enumerate(os.walk(src_dir)): | ||
| for filename in filenames: | ||
| if re.match(r"^[^.#~!$@%^&*()+=,]+\.html?$", filename): |
There was a problem hiding this comment.
Why the complicated regex? Yes, this would slightly be slower, but also more readable:
ignored, ext = os.path.splitext(filename)
if ext in (".htm", ".html"):
...
There was a problem hiding this comment.
Also I expect to make a comment in the coverage.py PR that find_unexcuted_files() should be a generator, so it should yield the matching files.
There was a problem hiding this comment.
The complicated regex was derived from here, in the find_python_files function.
I tried to keep the code corresponding with the rest of the code base, but I agree that the suggestion you make is a lot more readable, and to me atleast; preferable.
There was a problem hiding this comment.
I like the explanation in find_python_files for the regex, so (and I am sorry about this), you should probably put the regex back, but also include the comment from find_python_files as well.
|
I have incorporated the changes you suggested. |
django_coverage_plugin/plugin.py
Outdated
| return FileReporter(filename) | ||
|
|
||
| def find_executable_files(self, src_dir): | ||
| for i, (dirpath, dirnames, filenames) in enumerate(os.walk(src_dir)): |
There was a problem hiding this comment.
I don't think there's any reason to use enumerate here if you're not using i later on.
| from __future__ import print_function | ||
|
|
||
| import os.path | ||
| import re |
There was a problem hiding this comment.
Of course, I've just suggested putting it back (wince)
There was a problem hiding this comment.
Oops. Missed that conversation, sorry! 👍
There was a problem hiding this comment.
Nope, my conversation came after your reasonable comment. :)
|
I have incorporated the changes you suggested. |
|
I'll integrate this into my fork tonight (since my fork is connected to travis-ci) and assuming the tests pass, we'll merge this in. Before we can release it, we'll have to write a test. The unittest shouldn't be that hard, but an integration test might be tricky since the dj-coverage test suite isn't set up to test against the unreleased version of coverage.py. And since the feature isn't available in a released coverage.py, there's no immediate need to create a new release of dj-coverage (yet) |
|
Hi, Getting this upstream quickly isn't a priority for me. I'm currently just utilizing my own forks for my use, so take you time :) |
|
The new plugin method is in coverage.py 4.4b1. |
|
Shall we close the pull request then? |
|
I'd like to see a unit test for I'll merge this PR if you could add your name to AUTHORS.txt first. Then I'll update and increase the testing framework a bit. |
|
Hi @PamelaM, I added myself to the Is the unit test something you will do, or do you expect me to do it, after you add the |
|
Sorry - I was figuring out my intent as I typed and didn't re-read before I hit comment. It's not my best habit. I'll handle the unit test, though I'll poke you to review the PR, if that's okay. |
|
Thanks for the PR! |
|
Okay excellent. Just poke me to review whatever you'd like :) |
5 participants
This pull-request utilizes the API extension provided by:
To solve the challenge presented in:
Essentially, it now reports coverage for all
*.htmlfiles in the subtree it's run, which is arguably too wide, but a start for testing and discussion.