search

damnever / pigar

:coffee: A tool to generate requirements.txt for Python project, and more than that. (IT IS NOT A PACKAGE MANAGEMENT TOOL)
https://damnever.github.io/pigar/
BSD 3-Clause "New" or "Revised" License
1.67k stars 90 forks source link

support ipynb ipython parser for magic and system #88

Open yasirroni opened 3 years ago

yasirroni commented 3 years ago

Solve #87

pep8speaks commented 3 years ago

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

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

Comment last updated at 2021-02-21 09:18:18 UTC
yasirroni commented 3 years ago

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

Need feedback from author.

damnever commented 3 years ago

👍 Thanks for the contribution!

yasirroni commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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, notifications@github.com 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 https://github.com/damnever/pigar/pull/88#issuecomment-781783318, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALTUAKDKNIJUWHMEWHOQP4LS7XI67ANCNFSM4XQA6TNQ .

damnever commented 3 years ago

I will look into details.. 😫

yasirroni commented 3 years ago

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.

yasirroni commented 3 years ago

I design this version to:

  1. Allow the use of IPython >= 7
  2. Allow the use of IPython 5.x LTS and IPython 6.x (still not implemented, because I don't understand the doc).
  3. 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 commented 3 years ago

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 commented 3 years ago

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

https://github.com/damnever/pigar/pull/88#discussion_r579587904

damnever commented 3 years ago

All tests passed on GitHub CI.

yasirroni commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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, notifications@github.com 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 https://github.com/damnever/pigar/pull/88#issuecomment-782847373, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALTUAKHSMMOIWCYHHDFOKBLTADZRDANCNFSM4XQA6TNQ .

damnever commented 3 years ago

Anyway, thanks again.

yasirroni commented 3 years ago

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, notifications@github.com 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 https://github.com/damnever/pigar/pull/88#issuecomment-782858644, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALTUAKEKW3DGB22NXFNFY43TAEDJ7ANCNFSM4XQA6TNQ .

damnever commented 3 years ago

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..

1) 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.

2) 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 commented 3 years ago

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 commented 3 years ago

Two cases:

  1. IPython available: .ipynb file with magic marks should pass the test case
  2. 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 commented 1 year ago

Now, we are using regex to exclude magics and shell commands: https://github.com/damnever/pigar/pull/117

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