-
-
Notifications
You must be signed in to change notification settings - Fork 48
opt-in to cell magics #559
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
Conversation
|
@MarcoGorelli : I am will review this PR tomorrow. Could you keep this open till then? |
nbqa/save_source.py
Outdated
| if magic_type != IPythonMagicType.CELL: | ||
| return False | ||
| first_line = source[0].lstrip() | ||
| return all(first_line.split()[0] != f"%%{i}" for i in process) |
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.
all(first_line.split()[0] != f"%%{i}" for i in process) could be simplified as first_line.split()[0].lstrip("%%") not in process
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.
Thanks! Though note that .lstrip("%%") is the same as .lstrip("%"). I've changed this slightly as lstrip is really unintuitive, and using it resulted in bug #500
tests/tools/test_mypy_works.py
Outdated
| {path_3}:cell_8:3: error: Name 'flake8_version' is not defined | ||
| {path_3}:cell_8:4: error: Name 'flake8_version' is not defined | ||
| Found 5 errors in 4 files (checked 20 source files) | ||
| Found 5 errors in 4 files (checked 22 source 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.
Instead of updating this count every time, shall we do something like rglob on *.ipynb on the tests/data directory?
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.
yeah, definitely :) will keep that for a separate PR though as this one is big enough
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.
Sure, I just pointed out.
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 733 752 +19
Branches 106 110 +4
=========================================
+ Hits 733 752 +19
Continue to review full report at Codecov.
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #559 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 739 758 +19
Branches 106 110 +4
=========================================
+ Hits 739 758 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
closes #557