support ipynb ipython parser for magic and system #88

yasirroni · 2021-02-12T04:27:19Z

Solve #87

pep8speaks · 2021-02-12T04:50:12Z

Hello @yasirroni! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file pigar/parser.py:

Line 22:80: E501 line too long (82 > 79 characters)

Comment last updated at 2021-02-21 09:18:18 UTC

yasirroni · 2021-02-12T05:39:36Z

I don't understand what is making the error in the test.

Need feedback from author.

damnever · 2021-02-18T08:27:46Z

👍 Thanks for the contribution!

damnever · 2021-02-18T08:20:13Z

pigar/parser.py

+            transformer = IPython.core.inputtransformer2.TransformerManager()
+            for cell in nb.cells:
+                if cell.cell_type == "code":
+                    code += transformer.transform_cell(cell.source) + "\n"


Tests failed maybe because the transform_cell transformed the original code, such as adding empty lines.

damnever · 2021-02-18T08:23:48Z

pigar/parser.py

+    PYTHON_VERSION_3 = True
+    import IPython
+else:
+    PYTHON_VERSION_3 = False


What happens if Python 4.x gets released?

I think this is better:

try: import IPython _transformer_manager = IPython.core.inputtransformer2.TransformerManager() except Exception: # ??? _transformer_manager = None

Should I remove the comment? I don't have any idea for the comment nor the Exeption class.

except Exception:

pigar/parser.py

yasirroni · 2021-02-18T09:30:56Z

I escape the test. Need to add IPython >= 6.0.0 test. The relative import of your test file throws an error in my VSCode. I don't want to go to the bottom of the problem of the test for now. It just works in my machine (the IPython 7.0.0 transformer), I'm satisfied. :)

Would you like to add a test and let me know the diff of using IPython transformer before using Python parser and not using IPython transformer?

damnever · 2021-02-19T03:09:14Z

You know better what you are doing and I don't know how to write such a test for you, but I will try my best to help you.

So how did the transform_cell transformed the original code?

yasirroni · 2021-02-19T06:08:07Z

I don't know about that either. It's just what IPython use to transform %magic and !system into something like `Ipython.magic()`, so that it can be passed to Python parser. So yeah, it change the original code if it contains IPython syntax (%, !, ?, $). I think your test need to be edited, because your test making sure no code changing (i think).

…

On Fri, 19 Feb 2021, 10:09 Xiaochao Dong, ***@***.***> wrote: You know better what you are doing and I don't know how to write such a test for you, but I will try my best to help you. So how did the transform_cell transformed the original code? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#88 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALTUAKDKNIJUWHMEWHOQP4LS7XI67ANCNFSM4XQA6TNQ> .

damnever · 2021-02-19T06:36:25Z

I will look into details.. 😫

damnever · 2021-02-19T06:40:19Z

pigar/parser.py

-                code += cell.source + "\n"
+                if _transformer_manager:
+                    code += _transformer_manager.transform_cell(cell.source) \
+                        + "\n"


I think the problem is the + "\n", could you remove it and run the test to see what happens?

Ref:
https://github.com/ipython/ipython/blob/86d24741188b0cedd78ab080d498e775ed0e5272/IPython/core/inputtransformer2.py#L588-L597

yasirroni · 2021-02-19T13:42:38Z

I run test with python -m unittest discover -s tests, what is that with pyppeteer and syncer?

So yeah, I can't run your unittest in my machine.

damnever · 2021-02-20T02:52:17Z

pigar/parser.py

+    import IPython
+    # untested
+    # only support IPython >= 6.0.0
+    _transformer_manager = IPython.core.inputtransformer2.TransformerManager()


IPython should be present in setup.py,

Sadly, for the parser to works (my version), it requires IPython >= 6, whereas in colab, it's IPython 5. The problem, both versions have different Transformer API.

I remove IPython from the setup.py because it's not the core for pigar. It's just for pigar on IPython. If you want to make all support for IPython, that's good, but sadly I can't help because I can't run the test (can't debug your codes, i don't know why).

damnever · 2021-02-20T02:52:22Z

pigar/parser.py

+
+    # future: support IPython LTS 5.0.0 used by Google Colab
+    # _transformer_manager = ...
+


What does this mean?

It means that the current transformer (parser) only supports IPython > 6, and someone needs to add functionality to support IPython 5 LTS that is used by Google Colab.

I don't have IPython 5 LTS to test it and I don't use it anyway. So, Installing all the dependecy from scratch and make virtual environment is too much hassle (furthermore, I can't run your unittest).

None of them seems to be an issue to me, also to you.

You can run the tests like that:

pigar/makefile

Lines 10 to 11 in 823a3c2

run-tests:

python -m unittest discover pigar/tests/ -t . -v

Using IPython 7.0:

====================================================================== FAIL: test_py3_requirements (pigar.tests.test_core.ReqsTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "P:\GitHub\pigar\pigar\tests\test_core.py", line 64, in test_py3_requirements self.assertListEqual(sorted(guess.keys()), ['foobar']) AssertionError: Lists differ: ['foobar', 'subbar', 'subfoo'] != ['foobar'] First list contains 2 additional elements. First extra element 1: 'subbar' - ['foobar', 'subbar', 'subfoo'] + ['foobar'] ----------------------------------------------------------------------

Also, for IPython 5.0 LTS: transformer

I haven't figured out how to use it.

I asked stackoverflow, :)

yasirroni · 2021-02-20T09:58:51Z

I design this version to:

Allow the use of IPython >= 7
Allow the use of IPython 5.x LTS and IPython 6.x (still not implemented, because I don't understand the doc).
Without IPython (or we should not support this)?

Because there is exist the third option, a sub-test for IPython needs to be implemented. Your test cli must support if there is exist an IPython and not. Or should we make IPython as a requirements?

damnever · 2021-02-21T08:53:35Z

Without IPython (or we should not support this)?

Maybe we can try this: https://setuptools.readthedocs.io/en/latest/userguide/dependency_management.html#optional-dependencies, if I understand right so that users can do something like this: pip install pigar[extra-dep]

Allow the use of IPython 5.x LTS and IPython 6.x (still not implemented, because I don't understand the doc).

For now, I think you should put more details in the comment, such as TODO(username): xxx

yasirroni · 2021-02-21T09:19:50Z

Can you help me to solve this and write a test (also update the PyCl test).

#88 (comment)

damnever · 2021-02-21T10:35:50Z

All tests passed on GitHub CI.

yasirroni · 2021-02-21T10:40:09Z

All tests passed on GitHub CI.

To be honest, it's not passed. It's skipped (or evaded), because your test cli doesn't install IPython (which will evade my PR, and use the default non-IPython reader/transformer).

damnever · 2021-02-21T12:07:27Z

The failed test you posted here isn't related to IPython, you can run the tests in the master branch to see what happens. I have forgotten the reason.

BTW, you should make the CI to use IPython if you make it as an optional requirement.

yasirroni · 2021-02-21T12:57:53Z

It's related to IPython, in my case, as far as I know. If I run test without IPython, it works fine. Nah, am not understand CI at all. Haven't learnt about that. If you can't help me write the test, I think we can put a hold on this PR. Wait someone who want to pick this up or wait till I learnt about CI and how you wrote your test. Sadly, I'm not familiar with your test workflow.

…

On Sun, 21 Feb 2021, 19:07 Xiaochao Dong, ***@***.***> wrote: The failed test you posted here isn't related to IPython, you can run the tests in the master branch to see what happens. I have forgotten the reason. BTW, you should make the CI to use IPython if you make it as an optional requirements. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#88 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALTUAKHSMMOIWCYHHDFOKBLTADZRDANCNFSM4XQA6TNQ> .

damnever · 2021-02-21T13:30:52Z

Anyway, thanks again.

yasirroni · 2021-02-21T13:44:19Z

If you want to merge it to the master, that's good. The point is, it works flawlessly on my case, reading all my import in notebooks. I just don't understand your test and what makes my code not passing your test. but yeah, sadly, it doesn't work on google colab because of ipython 5 issue. Thank you for the experience.

…

On Sun, 21 Feb 2021, 20:31 Xiaochao Dong, ***@***.***> wrote: Anyway, thanks again. I am tired too. IMO, It is never about the tests or the ugly code, there is never an easy way to make things better. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#88 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALTUAKEKW3DGB22NXFNFY43TAEDJ7ANCNFSM4XQA6TNQ> .

damnever · 2021-02-21T14:19:05Z

Indeed, this project also lack of some documentations to help people to contribute, but this is also the part which people can contribute.. and I think that is what makes open-source great, and there are many things to be done, maintainers is not prefect as well.

And yes, I will not write every piece of code for contributors since that is.. I do not know how to describe it.. maybe insane..

As for this PR..

I think there is a misunderstanding for the "IPython5?" part:

Allow the use of IPython 5.x LTS and IPython 6.x (still not implemented, because I don't understand the doc).

For now, I think you should put more details in the comment, such as TODO(username): xxx

but yeah, sadly, it doesn't work on google colab because of ipython 5 issue.

We can ignore it for now, but for the people who are running into the same problem or interested in this, we should let them know why and where to find more contexts.
The workflow is just GitHub Actions, I do not know which part you do not understand exactly, I understand that you want to get it done, but that is never ok to me because I am maintaining this project, at least for now (not a good maintainer as well), and the tests is so important so that others won't break things.

yasirroni · 2021-02-24T09:00:56Z

After I disable IPython, running python -m unittest discover pigar/tests/ -t . -v also retrunt error

ine 64, in test_py3_requirements
    self.assertListEqual(sorted(guess.keys()), ['foobar'])
AssertionError: Lists differ: ['foobar', 'subbar', 'subfoo'] != ['foobar']

First list contains 2 additional elements.
First extra element 1:
'subbar'

- ['foobar', 'subbar', 'subfoo']
+ ['foobar']

It means that the error of the test is not from my modification? And why the local test fail but CI not fail? *really confused

Anyway,
If you want me to add test for IPython transformer, I don't know what should be tested? What is considered as failure? If Python ast.parse can't read it?

damnever · 2021-02-27T13:57:26Z

Two cases:

IPython available: .ipynb file with magic marks should pass the test case
IPython not available: ignore the .ipynb file with magic marks and skip case 1

You can start from here: https://github.com/damnever/pigar/blob/master/pigar/tests/test_core.py

Here we parse those files in test cases, and parse_packages can take an arguments to ignore directories, so you can put a .ipynb file with magic marks under some directory, skip test cases and ignore directory by condition.

damnever · 2022-11-07T13:54:50Z

Now, we are using regex to exclude magics and shell commands: #117

However, I think the approach in this PR is more elegant..

yasirroni added 3 commits February 12, 2021 11:23

support ipynb ipython parser for magic and system

6239181

abandon support for python 2 < 3.3, including 2.7

c951b89

allow python 2 without IPython parser support

d3c0ffb

fulfill pep8

a07d6b9

damnever requested changes Feb 18, 2021

View reviewed changes

escape ipython test, make ipython >= 6.0 an option

08c5fd8

follow request to follow common logic

73c1ac6

damnever requested changes Feb 19, 2021

View reviewed changes

yasirroni added 2 commits February 19, 2021 20:45

add .vscode in .gitignore

83c9069

remove add line after transform each cell

8e1dc0b

damnever requested changes Feb 20, 2021

View reviewed changes

damnever linked an issue Feb 21, 2021 that may be closed by this pull request

IPython % magic and ! exclamation mark throws error #87

Closed

yasirroni added 2 commits February 21, 2021 16:17

support optional ipython dependencies

0a58cfe

add TODO for future feature to support Ipython 5

568365a


		# future: support IPython LTS 5.0.0 used by Google Colab
		# _transformer_manager = ...

support ipynb ipython parser for magic and system #88

Are you sure you want to change the base?

support ipynb ipython parser for magic and system #88

Conversation

yasirroni commented Feb 12, 2021

pep8speaks commented Feb 12, 2021 • edited

Comment last updated at 2021-02-21 09:18:18 UTC

yasirroni commented Feb 12, 2021

damnever commented Feb 18, 2021

Choose a reason for hiding this comment

damnever Feb 18, 2021 • edited

Choose a reason for hiding this comment

yasirroni Feb 18, 2021 • edited

Choose a reason for hiding this comment

yasirroni commented Feb 18, 2021

damnever commented Feb 19, 2021

yasirroni commented Feb 19, 2021 via email

damnever commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirroni commented Feb 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirroni Feb 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasirroni commented Feb 20, 2021

damnever commented Feb 21, 2021

yasirroni commented Feb 21, 2021

damnever commented Feb 21, 2021

yasirroni commented Feb 21, 2021

damnever commented Feb 21, 2021 • edited

yasirroni commented Feb 21, 2021 via email

damnever commented Feb 21, 2021 • edited

yasirroni commented Feb 21, 2021 via email

damnever commented Feb 21, 2021 • edited

yasirroni commented Feb 24, 2021 • edited

damnever commented Feb 27, 2021

damnever commented Nov 7, 2022

pep8speaks commented Feb 12, 2021 •

edited

damnever Feb 18, 2021 •

edited

yasirroni Feb 18, 2021 •

edited

yasirroni commented Feb 19, 2021 •

edited

yasirroni Feb 20, 2021 •

edited

damnever commented Feb 21, 2021 •

edited

damnever commented Feb 21, 2021 •

edited

damnever commented Feb 21, 2021 •

edited

yasirroni commented Feb 24, 2021 •

edited