Skip to content

Fix/revisit get_package#176

Closed
blueyed wants to merge 1 commit intotwisted:masterfrom
blueyed:fix-get_package
Closed

Fix/revisit get_package#176
blueyed wants to merge 1 commit intotwisted:masterfrom
blueyed:fix-get_package

Conversation

@blueyed
Copy link
Copy Markdown
Contributor

@blueyed blueyed commented Mar 9, 2020

Try importing without changing sys.path first.

This makes a difference for pytest, where it would fail to
from _pytest._version import version, since _pytest._version does
not exist in the source tree (with non-editable installs), but only
the installed package.

It also improves the error reporting:

  • prefix with "ERROR" for what might be redirected
  • raise the ImportError directly (more information than when wrapped
    in Exception)

Ref: pytest-dev/pytest#6831

raise Exception("Can't find your project :(")
except ImportError:
# Step 1: Try the dumbest and simplest thing that could possibly work.
# Yes, that means importing it. Call the cops, I don't care.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've kept this comment as is, but might make sense to rephrase - there was never a step 1 for example.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a fixup (please squash-merge): it also fixes the sys.path.pop.

@hawkowl
Copy link
Copy Markdown
Member

hawkowl commented Mar 9, 2020

Codecov Report

Merging #176 into master will increase coverage by 0.04%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   89.54%   89.59%   +0.04%     
==========================================
  Files          11       11              
  Lines         440      442       +2     
  Branches       85       85              
==========================================
+ Hits          394      396       +2     
  Misses         25       25              
  Partials       21       21              
Impacted Files Coverage Δ
src/towncrier/_project.py 58.97% <44.44%> (+2.21%) ⬆️

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 fff788d...a7a0701. Read the comment docs.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 9, 2020

Codecov Report

Merging #176 into master will increase coverage by 0.04%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   89.54%   89.59%   +0.04%     
==========================================
  Files          11       11              
  Lines         440      442       +2     
  Branches       85       85              
==========================================
+ Hits          394      396       +2     
  Misses         25       25              
  Partials       21       21
Impacted Files Coverage Δ
src/towncrier/_project.py 58.97% <44.44%> (+2.21%) ⬆️

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 fff788d...8f607f2. Read the comment docs.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #176 into master will increase coverage by 0.09%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   89.49%   89.59%   +0.09%     
==========================================
  Files          11       11              
  Lines         438      442       +4     
  Branches       87       85       -2     
==========================================
+ Hits          392      396       +4     
  Misses         25       25              
  Partials       21       21              
Impacted Files Coverage Δ
src/towncrier/_project.py 58.97% <44.44%> (+2.21%) ⬆️
src/towncrier/_builder.py 93.60% <0.00%> (+0.10%) ⬆️

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 5c43102...2fb1315. Read the comment docs.

Try importing without changing `sys.path` first.

This makes a difference for pytest, where it would fail to
`from _pytest._version import version`, since `_pytest._version` does
not exist in the source tree (with non-editable installs), but only
the installed package.

It also improves the error reporting:

- prefix with "ERROR" for what might be redirected
- raise the `ImportError` directly (more information than when wrapped
  in `Exception`)

Ref: pytest-dev/pytest#6831
Copy link
Copy Markdown
Member

@altendky altendky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be reasonable to setup a test for this? It seems like a feature addition so a newsfragment would be useful? Thanks for the help here.

@altendky altendky mentioned this pull request Dec 13, 2020
@altendky
Copy link
Copy Markdown
Member

If you get back to this before I manage to find some reviews, feel free to merge #297 back in here and we can continue with this PR and close mine. I just didn't want to hijack your branch. Thanks again.

@blueyed
Copy link
Copy Markdown
Contributor Author

blueyed commented Dec 13, 2020

Cool, closing then.

@blueyed blueyed closed this Dec 13, 2020
@blueyed blueyed deleted the fix-get_package branch December 13, 2020 08:40
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.

5 participants