Closed Fernando-crz closed 11 months ago
tests failed due to nbconvert version, have you run the tests locally? Also, have you addressed the issue raised by @jamesmyatt about optional dependency?
I think @alan-barzilay means this issue: https://github.com/bndr/pipreqs/pull/210#discussion_r734564597
FWIW, black handles its Jupyter support via optional extras too: https://github.com/psf/black#installation
Attention: 2 lines
in your changes are missing coverage. Please review.
Comparison is base (
368e9ae
) 90.07% compared to head (0657a6d
) 90.17%.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Just fixed bugs related to the tests. The tests were failing because they were not considering that ipython was now part of the locally installed packages. Just fixed this issue now and tests are running just fine, but for some odd reason the codecov coverage report failed. @alan-barzilay
Just added support for jupyter notebook as an optional depency, as adressed by https://github.com/bndr/pipreqs/pull/210#discussion_r734564597. @alan-barzilay
Just fixed the process of installing jupyter notebooks as an optional dependency. Basically, the program detects if the user has the library nbconvert
installed, and if it is installed, the program will start detecting jupyter notebooks automatically when generating the requirements.txt file.
I was thinking of maybe turning the detection of jupyter notebooks when creating the requirements.txt file optional, using an optional argument with the name "--ignore-notebooks
". What do you think? @alan-barzilay
I was thinking of maybe turning the detection of jupyter notebooks when creating the requirements.txt file optional, using an optional argument with the name "
--ignore-notebooks
". What do you think? @alan-barzilay
yeah, good idea!
I was thinking of maybe turning the detection of jupyter notebooks when creating the requirements.txt file optional, using an optional argument with the name "
--ignore-notebooks
". What do you think? @alan-barzilay
Done.
Just so we have a written record of what we've just discussed: let's drop the idea of optional dependencies. Since we want the default option to include support for jupiter notebook, having optional dependencies don't bring us any value.
Besides that we also discussed the idea to further abstract some of the code processing steps to make them more auto contained and hide some of the logic with abstractions. We'll also hide the jupiter logic behind the global variable ignore notebooks, if it is set to true nbconverter will never be imported.
@Fernando-crz did i forget to mention anything?
@Fernando-crz can we close this PR if it is being continued in #414 ?
Just so we have a written record of what we've just discussed: let's drop the idea of optional dependencies. Since we want the default option to include support for jupiter notebook, having optional dependencies don't bring us any value.
Besides that we also discussed the idea to further abstract some of the code processing steps to make them more auto contained and hide some of the logic with abstractions. We'll also hide the jupiter logic behind the global variable ignore notebooks, if it is set to true nbconverter will never be imported.
@Fernando-crz did i forget to mention anything?
LGTM
@Fernando-crz can we close this PR if it is being continued in #414 ?
Sounds good to me.