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.

Sign up for GitHub

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

Jump to bottom

bpo-29546: Set path on ImportError for from ... import #91

Merged

brettcannon merged 3 commits into python:master from Carreau:betterimport

Feb 15, 2017

Contributor

Carreau commented Feb 14, 2017

And path as well when existing.

This is a step toward providing better error messages in case of
from-import. Barry Warsaw Proposed:

cannot import name {name} from {module} ({path})

But that's probably going to trigger more discussions that filling in
already existing fields.

I'm not familiar with C-internal of CPython, So let me know if I'm doing things wrong.

the-knights-who-say-ni added the CLA signed label

brettcannon reviewed

View reviewed changes

Python/ceval.c Outdated

    
                  PyErr_Format(PyExc_ImportError, "cannot import name %R", name);

                  pkgpath = PyModule_GetFilenameObject(v);

                  if (pkgpath == NULL || !PyUnicode_Check(pkgpath)){

Member

brettcannon Feb 14, 2017

Looks like there's a missing space between the final ) and {.

Contributor Author

Carreau Feb 14, 2017

Dang. Fixed.

Member

brettcannon commented Feb 14, 2017

I have not dug into the surrounding code, but the general approach LGTM.

Carreau force-pushed the betterimport branch from bd14cf2 to d3aff79 Compare

February 14, 2017 18:27

Member

brettcannon commented Feb 14, 2017

I also just realized there's no test.

brettcannon requested changes

View reviewed changes

Member

brettcannon left a comment

Please add a test.

Contributor Author

Carreau commented Feb 14, 2017

I also just realized there's no test.

Yes I realized that as well :-) Will do .

brettcannon requested changes

View reviewed changes

Lib/test/test_import/__init__.py Outdated

    
            @@ -80,6 +80,43 @@ def test_from_import_missing_attr_raises_ImportError(self):
          
                      with self.assertRaises(ImportError):

                          from importlib import something_that_should_not_exist_anywhere

                  def test_from_import_missing_attr_has_name_and_path(self):

                      """

Member

brettcannon Feb 14, 2017

We don't include docstrings on tests (and we PEP 8 doesn't allow starting a docstring with a newline).

Lib/test/test_import/__init__.py Outdated

    
                      except ImportError as e:

                          self.assertEqual(e.name, 'os',

                              "ImportError.name is set")

                          self.assertIsNotNone(e.path,

Member

brettcannon Feb 14, 2017

Could you do an assertEqual check using os.__file__?

Contributor Author

Carreau Feb 14, 2017

Sure.

Lib/test/test_import/__init__.py Outdated

    
                          from os import i_dont_exist

                      except ImportError as e:

                          self.assertEqual(e.name, 'os',

                              "ImportError.name is set")

Member

brettcannon Feb 14, 2017

The custom message isn't necessary as what the test is checking is obvious (same goes for all of the other messages).

Lib/test/test_import/__init__.py Outdated

    
                      `name` and `path` set when both are available.

                      """

                      try:

Member

brettcannon Feb 14, 2017

Any reason you can't use with self.assertRaises()?

Contributor Author

Carreau Feb 14, 2017

I just forgot you could access the exception after the fact with contextmanager.exception so I indeed I can ans should use it.

Lib/test/test_import/__init__.py Outdated

    
                      Make sure that from ... import ... raise an import error that has both

                      `name` and `path` set when both are available.

                      """

Member

brettcannon Feb 14, 2017

Unnecessary blank line.

Carreau force-pushed the betterimport branch 2 times, most recently from 3f218f5 to 30a5055 Compare

February 14, 2017 21:57

brettcannon requested changes

View reviewed changes

Lib/test/test_import/__init__.py Outdated

    
                      self.assertEqual(cm.exception.name, '_warning')

                      self.assertIsNone(cm.exception.path)

                  def test_from_import_missing_attr_path_is_cannonical(self):

Member

brettcannon Feb 14, 2017

Typo" cannonical" -> "canonical"

Contributor Author

Carreau Feb 14, 2017

I need to find a linter that understand both programming and english.

Lib/test/test_import/__init__.py Outdated

    
                  def test_from_import_missing_attr_path_is_cannonical(self):

                      with self.assertRaises(ImportError) as cm:

                          from os.path import i_dont_exist

                      self.assertIn(cm.exception.name, {'posixpath', 'ntpath'})

Member

brettcannon Feb 14, 2017

Should there be a test for path, even if it's just checking it isn't None?

Contributor Author

Carreau Feb 14, 2017

It could, but that's taken care of by functions above. Here I'm just checking that it's not os.path.
Anyway while I'm at it I will add an assertIsNotNone.

Lib/test/test_import/__init__.py Outdated

    
                      with self.assertRaises(ImportError) as cm:

                          from _warning import i_dont_exist

                      self.assertEqual(cm.exception.name, '_warning')

                      self.assertIsNone(cm.exception.path)

Member

brettcannon Feb 14, 2017

Could you add a comment like # _warning has no path as it's a built-in module.? I keep forgetting why this specific case doesn't have a path. 😄

Contributor Author

Carreau Feb 14, 2017

Too early in the bootstrap process ?
Anyway. Done.

Carreau force-pushed the betterimport branch from 30a5055 to 32e42aa Compare

February 14, 2017 22:39

codecov bot commented Feb 14, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@6de2b78). Click here to learn what that means.
The diff coverage is 64.28%.

@@            Coverage Diff            @@
##             master      #91   +/-   ##
=========================================
  Coverage          ?   82.38%           
=========================================
  Files             ?     1428           
  Lines             ?   351163           
  Branches          ?        0           
=========================================
  Hits              ?   289314           
  Misses            ?    61849           
  Partials          ?        0

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 6de2b78...32e42aa. Read the comment docs.

Member

berkerpeksag commented Feb 14, 2017

BTW, please rebase your branch so we can get rid of these Codecov comments :)

Carreau added 2 commits

February 14, 2017 14:48


          bpo-29546: Set name on ImportError for from ... import

ab2cb0e

And `path` as well when existing.

See bpo-29546

This is a step toward providing better error messages in case of
from-import. Barry Warsaw Proposed:

   cannot import name {name} from {module} ({path})

But that's probably going to trigger more discussions that filling in
already existing fields.


          Add test for the from ... import behavior.

7456cfc

Make sure they name and path are set in various case.

Carreau force-pushed the betterimport branch from 32e42aa to 7456cfc Compare

February 14, 2017 22:48

Contributor Author

Carreau commented Feb 14, 2017

BTW, please rebase your branch so we can get rid of these Codecov comments :)

Oh gosh 71 was merged ? Thanks you! and done.

brettcannon changed the title ~~bpo-29546: Set name on ImportError for from ... import~~ bpo-29546: Set path on ImportError for from ... import

brettcannon approved these changes

View reviewed changes

Member

brettcannon commented Feb 14, 2017

LGTM! Thanks for sticking through it, @Carreau !

Please add yourself to Misc/ACKS if you're not already there and add an entry into Misc/NEWS like:

bpo-29546: Set the 'path' attribute on ImportError for ``from ... import ...``.

Contributor Author

Carreau commented Feb 14, 2017

changed the title from bpo-29546: Set name on ImportError for from ... import to bpo-29546: Set path on ImportError for from ... import

Well actually good point that's setting both name and path. Name was not set either if that matters.

Please add yourself to Misc/ACKS if you're not already there and add an entry into Misc/NEWS like:

Sure !


          Add entry into Misc/NEWS

96b967a

Member

brettcannon commented Feb 14, 2017

Thanks! As soon as Travis is green I'll merge.

Contributor Author

Carreau commented Feb 15, 2017

Thanks! As soon as Travis is green I'll merge.

Thanks. I don't know how disturbing the migration has been from your end (and other core dev).
From my side being able to use git and PRs was great.

Many kudos for having worked on the migration for so long.

brettcannon merged commit bc4bed4 into python:master

Member

brettcannon commented Feb 15, 2017 •

edited

Loading

Merged! Thanks again!

And I'm glad that the new workflow worked out for you! As for me, the new workflow let me review this PR throughout the day when I had time and do it entirely in the browser so I didn't have to remember to commit it when I got home (if I had to cherry-pick that would not have been the case, but I could at least merge now and cherry-pick later if need be).

Carreau deleted the betterimport branch

February 15, 2017 00:08

Contributor Author

Carreau commented Feb 15, 2017

if I had to cherry-pick that would not have been the case

Hopping you will get the cherry-pick bot soon. Thanks for the time you've put into this.
I probably owe you a "orange juice, root beer, or Canada Dry," when we meet IRL.

Member

brettcannon commented Feb 15, 2017

@Carreau the bot would be great, but at worst me or someone else can write a Python script probably to automate it locally if necessary (which reminds me, I should write that idea down over on core-workflow).

Contributor Author

Carreau commented Feb 15, 2017

which reminds me, I should write that idea down over on core-workflow

You did python/core-workflow#8 (Bot to automatically generate cherry-picking PRs)

Member

warsaw commented Feb 15, 2017

Wow, I'm very pleasantly impressed at how quickly my bug/idea went to (nearly) implemented through the new workflow. Kudos!

Carreau mentioned this pull request

bpo-29546: Improve from-import error message with location #103

Merged

Contributor Author

Carreau commented Feb 15, 2017

Wow, I'm very pleasantly impressed at how quickly my bug/idea went to (nearly) implemented through the new workflow. Kudos!

Happy to repay a bit for all the work you did.

It's also much easier to decide to implement if a core dev have pitched in the idea, as it has more chance to be accepted. Thanks both !

Carreau added a commit to Carreau/cpython that referenced this pull request


          bpo-29546: Improve from-import error message with location

a181463

Add location information like canonical module name where identifier
cannot be found and file location if available.

First iteration of this was pythongh-91

akruis pushed a commit to akruis/cpython that referenced this pull request


          Stackless issue python#91: unbinding a tasklet does not reset the rec…

b6af5e9

…ursion depth

Reset the recursion depth in PyTasklet_BindEx and add a few asserts.
This fixes issue python#91.
Add a test case for the recursion depth and as test case for an assertion failure caused by this bug.

https://bitbucket.org/stackless-dev/stackless/issues/91
(grafted from 1584a89fca366f66dadf3cf04f0306dd45b1372f and 380b70b1933d)

jaraco pushed a commit that referenced this pull request


          Update celery from 4.1.0 to 4.1.1 (#91)

8ef6673

gvanrossum mentioned this pull request

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

ngoldbaum mentioned this pull request

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

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

Labels

None yet