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

Use pathlib for file path management #16

Closed
wants to merge 3 commits into from

Conversation

hectormz
Copy link

@hectormz hectormz commented May 6, 2020

This PR uses pathlib for file path management, which should make cross-platform support easier.

This PR resolves #15

pathlib could be implemented to replace a lot of os usage elsewhere in the project, but this PR was focused on coverage.py.

ATTN: @econchick

The original tests don't run properly on Windows, but this was tested and run on Linux, where all tests passed.

The default common_base = "" is now common_base = Path("/"). I'm not sure if this is fine with you, or if this breaks something in your use case.

Tests were updated, but hopefully they still test the properties you intended.

Let me know if there is anything that is not quite right, or some lines that could use some explanation. There are still some cases where a Path was converted into a str or where os was used. There may be a more elegant solution that I'm not familiar with.

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #16   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          509       504    -5     
  Branches       108       107    -1     
=========================================
- Hits           509       504    -5     
Impacted Files Coverage Δ
src/interrogate/cli.py 100.00% <100.00%> (ø)
src/interrogate/coverage.py 100.00% <100.00%> (ø)
src/interrogate/utils.py 100.00% <100.00%> (ø)
src/interrogate/visit.py 100.00% <0.00%> (ø)
src/interrogate/config.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aed1a40...8fba5a7. Read the comment docs.

@hectormz
Copy link
Author

hectormz commented May 6, 2020

Ah, I believe os.commonpath() was modified in 3.6 to accept Path objects instead of just str objects

@hectormz
Copy link
Author

hectormz commented May 8, 2020

@econchick So I've looked at the py3.5 failures a bit, and I think that although pathlib was introduced in 3.5, it started being treated differently in 3.6+ (accepted as an argument to open files etc), so more work will be needed to be done to make it 3.5 compatible.

@econchick
Copy link
Owner

@hectormz I'll take a look today and work off your branch. Thank you for doing 99% of the legwork! I really appreciate it.

@econchick econchick self-assigned this May 9, 2020
@hectormz
Copy link
Author

hectormz commented May 9, 2020

Awesome! Let me know if I can clear up anything I did that's not quite right.

@econchick
Copy link
Owner

Closing in favor of #18

@econchick econchick closed this May 11, 2020
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.

Bug: common_base not found on Windows
2 participants