search

lark-parser / lark_cython

Cython plugin for Lark, reimplementing the LALR parser & lexer for better performance
MIT License
44 stars 5 forks source link

Updated test_basic.py #35

Open Akshat111111 opened 4 months ago

Akshat111111 commented 4 months ago

For clarity, I have enhanced the assertion messages and provided informative docstrings. These tests concentrate on certain elements, encouraging readability and maintainability. Using pytest, the script runs without any difficulties. Every test carries out precise validations, improving understanding and debugging capacities for next development.

erezsh commented 4 months ago

Did you use chatgpt for this?

Akshat111111 commented 4 months ago

Did you use chatgpt for this?

No, Not at all

Akshat111111 commented 4 months ago

Can you please guide me in depth about where I m lagging

erezsh commented 4 months ago

I don't understand why you find these comments and docstrings important. For the most part you're just writing the test name with spaces instead of underscores..

Also, why add these comments to the tests? It's very simple code, and almost nobody reads it anyway.

But the real problem for me is that you changed the indentation for the entire file, instead of only adding lines.

Akshat111111 commented 4 months ago

Sorry for it, I will definately take care of such thing in future.

Akshat111111 commented 4 months ago

Can I do something else inorder to merge this code ??

Erotemic commented 4 months ago

I'm going to try my best at a constructive comment with the assumption that the author is working to improve as an open source contributor.

This set of changes isn't adding much value to the test suite. The comment describing the commit itself doesn't make any sense. In most cases the docstrings don't add new information, although there is an argument the ones for test_start and test_no_placeholders offer improved readability, the others don't. The PR adds pytest as an import, which is used to let the file run as an executable, but it also adds an import that is otherwise unneeded. Personally, I do prefer the 4 space indentation, and I think a separate PR to normalize that over the entire repo would be a good idea, but it does mess with git blame and I don't want to start a religious war.

It seems to me that the author is mainly incentivized by having a commit rather than improving the library. There's nothing necessarily wrong with the former - I like having my name on repos too - but the latter needs to be the primary driver. I recommend thinking more about the amount of library-value added compared to the number of line changes added to the git log and that need to be reviewed by maintainers.

Akshat111111 commented 4 months ago

I'm going to try my best at a constructive comment with the assumption that the author is working to improve as an open source contributor.

This set of changes isn't adding much value to the test suite. The comment describing the commit itself doesn't make any sense. In most cases the docstrings don't add new information, although there is an argument the ones for test_start and test_no_placeholders offer improved readability, the others don't. The PR adds pytest as an import, which is used to let the file run as an executable, but it also adds an import that is otherwise unneeded. Personally, I do prefer the 4 space indentation, and I think a separate PR to normalize that over the entire repo would be a good idea, but it does mess with git blame and I don't want to start a religious war.

It seems to me that the author is mainly incentivized by having a commit rather than improving the library. There's nothing necessarily wrong with the former - I like having my name on repos too - but the latter needs to be the primary driver. I recommend thinking more about the amount of library-value added compared to the number of line changes added to the git log and that need to be reviewed by maintainers.

Thankyou so much for such a detailed description.I will necessary follow all the above instructions mentioned by you.

Erotemic commented 4 months ago

I'm sorry if I'm wrong, but I am getting strong LLM vibes here.

erezsh commented 4 months ago

@Erotemic That's what I thought :) It's only the spelling mistakes that make me wonder.

Anyway, I just ran black over everything, so indentation fixed. No need for religious wars, we have enough of those in RL ;)