carpentries-incubator / python-packaging-publishing

Packaging and Publishing with Python
https://carpentries-incubator.github.io/python-packaging-publishing/
Other
24 stars 16 forks source link

03-packaging-installing: Improving episode. #77

Open vinisalazar opened 3 years ago

vinisalazar commented 3 years ago

Hi,

there are some things I'd suggest to improve Ep03:

- Fix the ModuleNotFoundError

If I run the code in the current version of Episode 03, when I run the following line:

from conversions import temperature, speed

Assuming I'm in the conversions/ directory as instructed, I get the following error:

---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-4-180d6e73763d> in <module>
----> 1 from conversions import temperature, speed

ModuleNotFoundError: No module named 'conversions'

This does work, if I'm in the conversions/ directory:

from temperature import fahr_to_celsius
from speed import kph_to_ms

Not entirely sure if I am doing anything wrong.

If I open a Python terminal from the parent directory of conversions and run import sys; sys.path.append('conversions'), which is explained earlier in the episode, I can run the imports as they are currently displayed (from conversions import temperature, speed).

- Move the Pip section to somewhere else in the Episode

As described in #58, the Pip section seems a bit out of place. I'd suggest moving it somewhere else in the Episode.

I will document other things I find in this issue. May I submit a PR to address these two points in the meantime?

brownsarahm commented 3 years ago

You shouldn't be in the conversion directory, but in the top level directory.

Can you address moving pip in a separate PR from other changes?

vinisalazar commented 3 years ago

You shouldn't be in the conversion directory, but in the top level directory.

I agree. However, the episode states:

Now, if we launch a new python terminal from this directory, we can import the package conversions

Since the conversions/ directory is being discussed, it does sound a bit ambiguous in the sense of "this directory" being conversions/, rather than the top level directory. If I am in the top level directory, I get a different error:

---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
<ipython-input-1-180d6e73763d> in <module>
----> 1 from conversions import temperature, speed

~/Bio/carpentries/python-packaging-publishing/code/conversions/__init__.py in <module>
----> 1 import temperature
      2 import speed

ModuleNotFoundError: No module named 'temperature'

This is fixed by running the command sys.path.append('conversions'). The imports run with no problems after that.

Can you address moving pip in a separate PR from other changes?

Sure, will do.

Edit: apparently the from conversions import temperature, speed line works from the top level directory in the absence of the __init__.py file (the error actually traces back to it). I'll try to see what's the best way to present this; my main concern is making sure the sequence of commands in the episode can be reproduced without any errors.

brownsarahm commented 3 years ago

ahhh, sorry. this got changed a couple of times and I hadn't read carefully.

I think the goal should be to first introduce importing a function from a module while in the same directory, without adding a path. So, from conversions/

from temperature import fahr_to_celsius

then add more functions, try to import from the top level, see that you need something more and introduce install.

I know that we can add to path without installing, but i think that's a suboptimal thing and we should skip that versino.

vinisalazar commented 3 years ago

I pushed #78 addressing the observations from the last comment.

This StackOverflow issue explains the reasons for the errors (it is necessary to use local imports).

Edit: thanks for the quick reply. It seems we commented at practically the same time :)