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

fix nbdev_update #1058

Merged
merged 2 commits into from
Sep 14, 2022
Merged

fix nbdev_update #1058

merged 2 commits into from
Sep 14, 2022

Conversation

seeM
Copy link
Member

@seeM seeM commented Sep 14, 2022

Apologies for the huge PR! I'm happy to break it down into smaller PRs if you prefer

  • fix bug in nbdev.maker.update_import which meant that nbdev_update didn't convert relative imports without None module (e.g from . import foo -> from pkg import foo)
  • fix FileNotFoundError in nbdev_update by passing the correct py module and corresponding notebook paths
    • refactor nbdev.doclinks._get_modidx to use extracted _iter_py_cells function to support this
  • fix nbdev_update introducing whitespace changes to notebooks
    • refactor nbdev.process.extract_directives to use extracted _partition_cell function to support this

- refactor `nbdev.doclinks._get_modidx` to use extracted `_iter_py_cells` function.
- fix bug in `nbdev.maker.update_import` which meant that `nbdev_update` didn't convert relative imports without `None` module (e.g `from . import foo` -> `from pkg import foo`)
- refactor `nbdev.process.extract_directives` to use extracted `_partition_cell` function.
- fix `FileNotFoundError` in `nbdev_update` by passing the correct py module and corresponding notebook paths
- fix `nbdev_update` introducing whitespace changes to notebooks
@seeM seeM added the bug Something isn't working label Sep 14, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

def _nbpath2html(p): return p.with_name(re.sub(r'\d+[a-zA-Z0-9]*_', '', p.name.lower())).with_suffix('.html')

# %% ../nbs/api/doclinks.ipynb 13
def _get_modidx(py_path, code_root, nbs_path):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should not change behaviour at all. They're to use the new _iter_py_cells function which is also used in sync.ipynb

@@ -149,7 +149,7 @@ def update_import(source, tree, libname, f=relative_import):
nmod = f(imp.module, libname, imp.level)
lin = imp.lineno-1
sec = src[lin][imp.col_offset:imp.end_col_offset]
newsec = re.sub(f"(from +){'.'*imp.level}{imp.module}", fr"\1{nmod}", sec)
newsec = re.sub(f"(from +){'.'*imp.level}{imp.module or ''}", fr"\1{nmod}", sec)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to fix:

fix bug in nbdev.maker.update_import which meant that nbdev_update didn't convert relative imports without None module (e.g from . import foo -> from pkg import foo)

def extract_directives(cell, remove=True, lang='python'):
"Take leading comment directives from lines of code in `ss`, remove `#|`, and split"
if cell.source:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should not change behaviour at all. It's to export _partition_cell which is used in sync.ipynb

@@ -8,7 +8,9 @@
from .config import *
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this by running nbdev_update in the nbdev repo and ensuring that it didn't change any of our notebooks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

@seeM seeM requested review from jph00 and hamelsmu September 14, 2022 08:44
@@ -43,35 +43,43 @@ def patch_name(o):
return _sym_nm(a,o)

# %% ../nbs/api/doclinks.ipynb 9
def _nbpath2html(p): return p.with_name(re.sub(r'\d+[a-zA-Z0-9]*_', '', p.name.lower())).with_suffix('.html')
def _iter_py_cells(p):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach!

@jph00
Copy link
Member

jph00 commented Sep 14, 2022

Looks great!

@jph00 jph00 merged commit b4cd2ac into fastai:master Sep 14, 2022
@seeM seeM deleted the fix-sync branch September 14, 2022 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants