Skip to content

clear all is not allowed #6

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

Open
asereq opened this issue Feb 15, 2022 · 4 comments
Open

clear all is not allowed #6

asereq opened this issue Feb 15, 2022 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@asereq
Copy link

asereq commented Feb 15, 2022

Hi!
Great work, thanks for the package!

It seems like if the notebook has a "clear all" line,
the "clear" clean variables of the class itself:

  error: 'obj' undefined near line 545, column 545
  error: called from
      evalCode at line 545 column 7
      run at line 389 column 20

My workaround is to delete the line before the execution, but I think it could be a better solution.
Thanks again!

@siko1056 siko1056 added the bug Something isn't working label Feb 17, 2022
@siko1056
Copy link
Member

Thanks for reporting the issue. @Abdallah-Elshamy do you have an idea how to deal with it?

@siko1056 siko1056 pinned this issue Feb 17, 2022
@siko1056 siko1056 unpinned this issue Feb 17, 2022
@Abdallah-Elshamy
Copy link
Member

Thanks for the report!

@siko1056 , Since clear all removes all the stored variables, it also removes the jupyter_notebook object itself. When the evalCode function tries to save the context after that, the error happens.

A rough solution for that would be detecting the clear all or clear and add the -x option to it to prevent the object from being deleted. However, this will still affect the variables defined outside of the notebook.

To provide better isolation for the notebook variables, it is better to parse the clear commands and remove the deleted variable from the context that we are storing in the jupyter_notebook object. However, this parsing will slow down performance. There may be also other options that I am not considering. What do you think?

siko1056 added a commit that referenced this issue Feb 18, 2022
@siko1056
Copy link
Member

siko1056 commented Feb 18, 2022

Thanks for the brainstorming @Abdallah-Elshamy . I also thought about parsing for clear, but I think there are too many ugly cases of calling clear to be considered, this might not be a stable solution.

I had a very elegant solution, see #7. However, there is https://savannah.gnu.org/bugs/?62077 😓 Declaring everything as public as last resort violates OOP a lot 😅

@Abdallah-Elshamy
Copy link
Member

Thanks for the brainstorming @Abdallah-Elshamy . I also thought about parsing for clear, but I think there are too many ugly cases of calling clear to be considered, this might not be a stable solution.

I agree. Too many edge cases to handle and even if we got all of them right, the performance would be worse.

I had a very elegant solution, see #7.

Neat!

However, there is https://savannah.gnu.org/bugs/?62077 😓 Declaring everything as public as last resort violates OOP a lot 😅

I don't think that will be a good idea too. I believe that maintaining OOP principles is more important than handling this edge case. I guess this will have to wait until this bug is solved 😓.

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

No branches or pull requests

3 participants