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

Catch FileNotFoundError in config.py - issue #384 #385

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

indera
Copy link

@indera indera commented Aug 23, 2016

[maintainer edit - this is re #384. github please parse subjects for references sometime ugggh]

@codecov-io
Copy link

codecov-io commented Aug 23, 2016

Codecov Report

Merging #385 into master will decrease coverage by 0.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #385     +/-   ##
=========================================
- Coverage    93.5%   92.91%   -0.6%     
=========================================
  Files          21       20      -1     
  Lines        2141     1793    -348     
  Branches      377      307     -70     
=========================================
- Hits         2002     1666    -336     
+ Misses        100       93      -7     
+ Partials       39       34      -5
Impacted Files Coverage Δ
invoke/config.py 90.58% <100%> (-4.11%) ⬇️
invoke/exceptions.py 68.85% <0%> (-19.39%) ⬇️
invoke/parser/argument.py 91.42% <0%> (-1.6%) ⬇️
invoke/runners.py 89.93% <0%> (-1.59%) ⬇️
invoke/platform.py 66.1% <0%> (-1.12%) ⬇️
invoke/util.py 87.27% <0%> (-1.07%) ⬇️
invoke/__init__.py 100% <0%> (ø) ⬆️
invoke/watchers.py
invoke/program.py 98.66% <0%> (+0.82%) ⬆️
... and 7 more

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 24570f8...27ecc01. Read the comment docs.

@PatrickMassot
Copy link

It's very nice to fix #384 but why did you drop python 2.6 in the same PR??

Have you seen #364?

@bitprophet
Copy link
Member

Thanks for this (& raising the issue itself in #384 - sadly I don't have the time nor expertise to have full international support in my initial code :D). Some feedback:

  • As @PatrickMassot notes we still support 2.6 pending Consider dropping/deprecating Python 2.6 support #364, so please make sure the PR works there :)
  • What's .ropeproject? I generally prefer people use their own gitignores and not the project's gitignore, for anything the project itself doesn't require for development. So dropping that part of the patch would be appreciated too.
  • I'm slightly torn on the actual, core fix: obviously introspecting the error message for English is non-ideal, but I worry expanding the scope to simply "any OSError or IOError" could hide other subtypes of such errors.
    • Part of me wonders if it's worth testing specifically for FileNotFound on Python 3; but the syntactical overhead of doing that probably isn't worth it (until/when we drop Python 2).
    • Overall, though, the chance of some error at this point that is meaningfully different from "file not found" feels low, and users in those situations who see behavior they're not expecting, will hopefully enable logging. So yea. I'll probably merge this :)

+ remove user specific entry in .gitignore
(@see rope -- https://github.com/klen/python-mode)
@indera
Copy link
Author

indera commented Aug 26, 2016

@bitprophet .ropeproject/ folder is generated by a vim python plugin I am using: https://github.com/klen/python-mode

@indera
Copy link
Author

indera commented Aug 30, 2016

Fails on windows but other than that looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants