-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add support for jupyter notebook #414
Conversation
e37ba5e
to
6b0d760
Compare
6b0d760
to
ed82776
Compare
ed82776
to
3b16e0b
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## next #414 +/- ##
==========================================
+ Coverage 90.07% 90.16% +0.09%
==========================================
Files 2 2
Lines 262 295 +33
==========================================
+ Hits 236 266 +30
- Misses 26 29 +3 ☔ View full report in Codecov by Sentry. |
3b16e0b
to
932deea
Compare
We rebased this branch into the next branch to add the poetry changes. After that, we added a flag Also, we also did a change in the tests to suppress all the logged warnings and errors during test execution. At last, we updated the |
0f28659
to
25b1722
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work, but too much cross contamination in commits. Also, tests are failing, where you expecting them to pass?
tests/test_pipreqs.py
Outdated
""" | ||
Test the function ipynb_2_py() which converts .ipynb file to .py format | ||
""" | ||
expected = pipreqs.get_all_imports(self.compatible_files["original"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is self.compatible_files["original"]? a list of expected imports? if so, this is not a good intuitive name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. self.compatible_files
is a dictionary containing two keywords: original
and notebook
. The items of the dictionary contain paths to files that have the same imports, but one has the .py extension and other has the .ipynb extension.
This test basically converts both files and check if their imports are the same.
201e6c0
to
ef26256
Compare
3fc7d66
to
a0a44c1
Compare
@alan-barzilay @mateuslatrova Just rebased all the changes, fixing possible cross-contamination errors. Please, feel free to check my changes to make sure everything is ok. Also fixed other changes asked by Alan. Now, we only need to check if |
Actually, considering the coverage error was fixed by adding the installation of |
just for the sake of completion, our working hypothesis is that ipython is necessary to parse magic commands in notebooks, our tests were failing due to a parsing error involving magics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost done! the next version will probably be the final one!
walk = os.walk(path, followlinks=follow_links) | ||
for root, dirs, files in walk: | ||
dirs[:] = [d for d in dirs if d not in ignore_dirs] | ||
|
||
candidates.append(os.path.basename(root)) | ||
files = [fn for fn in files if os.path.splitext(fn)[1] == ".py"] | ||
py_files = [file for file in files if file_ext_is_allowed(file, [".py"])] | ||
candidates.extend([os.path.splitext(filename)[0] for filename in py_files]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fernando-crz @mateuslatrova help me sanity check my understanding of the files
and candidates
objects, files will be parsed and processed to get the list of imported packages, but this list will be contaminated by stdlib packages and local imports which will be removed in other processing steps (lines 165 and 171).
The candidates list is simply a list of local files that could be imported (like an utils.py file) but shouldnt be a part of the requirements file. Is that it? If so, isnt candidates an awful name? what are they candidates of? also, this would explain why we are adding dir by dir to the list, as I raised in issue #424 and on our last meeting.
Maybe we could take this opportunity to rename those objects to something more intuitive? like local_files or local_modules since dirs can also be imported.
If you think this should be in another pr, ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
py_files -> local_files
candidates -> local_modules
(pretty sure thts the correct nomenclature but we could use a double check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just leave it for another PR? 🫥
We are not completely sure what is going on with those variables. We just copied the logic as it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fernando-crz my dude, you gotta understand whats going on and not just blindly reuse code haha
i opened issue #427 to keep track of this
a0a44c1
to
61bc4a0
Compare
Credits to @mateuslatrova for the contribution.
61bc4a0
to
97a0f5e
Compare
Credits to @pakio and @mateuslatrova for the contributions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an important and long coming PR, thank you all for the work! I' m finally comfortable with merging this
walk = os.walk(path, followlinks=follow_links) | ||
for root, dirs, files in walk: | ||
dirs[:] = [d for d in dirs if d not in ignore_dirs] | ||
|
||
candidates.append(os.path.basename(root)) | ||
files = [fn for fn in files if os.path.splitext(fn)[1] == ".py"] | ||
py_files = [file for file in files if file_ext_is_allowed(file, [".py"])] | ||
candidates.extend([os.path.splitext(filename)[0] for filename in py_files]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fernando-crz my dude, you gotta understand whats going on and not just blindly reuse code haha
i opened issue #427 to keep track of this
This branch is a continuation of PR #404. Copied it to the pipreqsxp fork so that the other members can also contribute.