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

03-packaging-installing: Improving episode. #77

Open
vinisalazar opened this issue May 11, 2021 · 4 comments
Open

03-packaging-installing: Improving episode. #77

vinisalazar opened this issue May 11, 2021 · 4 comments

Comments

@vinisalazar
Copy link
Contributor

vinisalazar commented May 11, 2021

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
Copy link
Collaborator

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
Copy link
Contributor Author

vinisalazar commented May 13, 2021

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.

vinisalazar added a commit to vinisalazar/python-packaging-publishing that referenced this issue May 13, 2021
  - Related to carpentries-incubator#77
  - Fixes the sequence of commands (does not return errors anymore) and improves the explanation of the init file.
@vinisalazar vinisalazar mentioned this issue May 13, 2021
2 tasks
@brownsarahm
Copy link
Collaborator

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
Copy link
Contributor Author

vinisalazar commented May 13, 2021

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 :)

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

No branches or pull requests

2 participants