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

Compatibility with jupyter notebooks #31

Merged
merged 17 commits into from
Sep 18, 2023
Merged

Compatibility with jupyter notebooks #31

merged 17 commits into from
Sep 18, 2023

Conversation

rraadd88
Copy link
Contributor

@rraadd88 rraadd88 commented Mar 22, 2023

Steps:

  1. Input notebook read using nbformat,
  2. converted to python script using nbconvert,
  3. removestar to find replacements,
  4. necessary replacements done in the notebook's code cells and
  5. the modified notebook saved using nbformat.

Apart from core functionalities inherited from removestar:

  1. Allows in-place editing and
  2. allows saving modified file with separate filename using the -o command line argument.
  3. replace_imports function returns a dictionary with string replacements, using return_replacements flag. This would allow further extensions as needed.
  4. notebook specific extra requirements could be installed using pip install removestar[nb]

Not added:

  1. Testing functions.

@asmeurer
Copy link
Owner

Does nbconvert do a roundtrip conversion of notebooks? I'm worried some metadata in the notebooks might be lost if we do this.

@rraadd88
Copy link
Contributor Author

AFIK, nbconvert can not do that. Only jupytext can but in my experience, the 'roundtripped' notebook was not the same as the original one.

In my code, .py file is not converted back to the notebook. Rather, string replacements were done only in the code-cells of the original notebook. Other fields of the notebook e.g. metadata are not operated on.

@asmeurer
Copy link
Owner

Oh I see. That seems better then.

It would be good to have at least one test of this. Maybe you can generate an example before and after notebook file and check that it converts the one to the other.


def replace_imports(code, repls, *, max_line_length=100, file=None, verbose=False, quiet=False):
def replace_imports(code, repls, *, max_line_length=100, file=None, verbose=False, quiet=False,return_replacements=False):
Copy link
Owner

Choose a reason for hiding this comment

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

Keyword arguments that change the return type aren't great. It would be better to just have a separate function to return replacements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look again. As I see it, refactoring replace_imports for removestar_nb might be problematic because it would need duplicating the code. I agree that the keyword arguments that change the return type are not ideal. But I am not sure if there's any alternative here.

@rraadd88
Copy link
Contributor Author

Oh I see. That seems better then.

It would be good to have at least one test of this. Maybe you can generate an example before and after notebook file and check that it converts the one to the other.

I just added a test for removestar_nb.
It is developed on top of removestar, so I am assuming that it might not be necessary to re-test the base code for the context where notebook is provided as an input. But please let me know if some tests might be needed.

@rraadd88
Copy link
Contributor Author

I hope the test I added for removestar_nb was sufficient.

@kieran-ryan kieran-ryan added the enhancement New feature or request label Sep 4, 2023
@Saransh-cpp Saransh-cpp self-requested a review September 4, 2023 20:01
removestar/__main__.py Outdated Show resolved Hide resolved
removestar/removestar.py Outdated Show resolved Hide resolved
removestar/removestar.py Outdated Show resolved Hide resolved
@Saransh-cpp
Copy link
Collaborator

I've refactored almost the whole PR while keeping the original logic intact, but the code is still very repetitive and can be refactored more. The code now looks similar to the old code and everything works now, the new tests pass and the old ones are passing too.

I think it should be okay to merge this in, make a new release, and then iteratively work on refactoring the code.

Thanks for taking this up, @rraadd88! I'll maybe let this sit for a few days and then merge (I might end up gathering the courage to refactor this further over the weekend).

@Saransh-cpp Saransh-cpp merged commit 9c70ab4 into asmeurer:master Sep 18, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants