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

Stop creating unused variables #68

Merged

Conversation

0dminnimda
Copy link
Contributor

No description provided.

@0dminnimda
Copy link
Contributor Author

0dminnimda commented Jul 3, 2022

Is it reasonable to first try to merge it to cpython?

@pablogsal
Copy link
Contributor

Is it reasonable to first try to merge it to cpython?

Generally we do it the other way: we merge stuff here and I sync them from time to time in batches.

@0dminnimda
Copy link
Contributor Author

0dminnimda commented Jul 3, 2022

Ok, I see.
Also can I ask you for the launch of the workflow, please?

@0dminnimda
Copy link
Contributor Author

Probably a good time to say, that two tests are failing on the main. The same tests are failing here. Although I think fixing those tests is out of the scope of this pr, unfortunately

@MatthieuDartiailh
Copy link
Collaborator

Getting #66 in first would make sense to get CIs to test on more versions.

@0dminnimda
Copy link
Contributor Author

0dminnimda commented Jul 4, 2022

Does #66 fixes broken tests? If yes, then we probably should try merging those fixes first independently of the #66.

@0dminnimda
Copy link
Contributor Author

Generally we do it the other way: we merge stuff here and I sync them from time to time in batches

Well, then what's about distributing credit/blame for the changes? Will those changes be cherry-picked into cpython?

@pablogsal
Copy link
Contributor

pablogsal commented Jul 4, 2022

Generally we do it the other way: we merge stuff here and I sync them from time to time in batches

Well, then what's about distributing credit/blame for the changes? Will those changes be cherry-picked into cpython?

No, they will be directly included in one batch and blame/credit is lost.

@0dminnimda
Copy link
Contributor Author

No, they will be directly included in one batch and blame/credit is lost.

Aww, isn't this a reason to make a direct pr to cpython?

Well, not like it's that important, but it would be nice if the credit/blame was preserved!

@pablogsal
Copy link
Contributor

Aww, isn't this a reason to make a direct pr to cpython?

No, we don't generally merge individual PRs in CPython for pegen because it makes syncing much more complicated.

Well, not like it's that important, but it would be nice if the credit/blame was preserved!

Well, the canonical code for pegen is here. The CPython version doesn't match this code because it has a bunch of Cpython-isms and the C generator which is is own beast. Notice that the only python parser being generated in CPython is the grammar parser, but the main output is the C version.

@MatthieuDartiailh
Copy link
Collaborator

@0dminnimda Can you rebase on main so that we get an extended test run ?

@0dminnimda
Copy link
Contributor Author

Oh yeah, workflow activation, what a lovely github feature ♥

@gvanrossum
Copy link

Bye, I am leaving this repo now.

@0dminnimda
Copy link
Contributor Author

oh .. bye




well, we need to get back to the work

3.10 didn't pass, it seems like versoin of python that tox runs is somehow different from the other ones
Because if I try replicate this I see a string without '

I don't quite understand why it happens

@MatthieuDartiailh
Copy link
Collaborator

#71 will fix the failure on 3.10 (3.10.5 fixed an issue for which we had a workaround)

@0dminnimda
Copy link
Contributor Author

Closing and opening to triger the ci

@0dminnimda 0dminnimda closed this Jul 16, 2022
@0dminnimda 0dminnimda reopened this Jul 16, 2022
@0dminnimda
Copy link
Contributor Author

oh yeah, forgot to merge

@0dminnimda
Copy link
Contributor Author

Is there any additional comments or this can be merged?

Copy link
Collaborator

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

I found one concerning issue regarding cut (and I find even more concerning that no test found the issue). I also left a couple of comment/question.

src/pegen/python_generator.py Outdated Show resolved Hide resolved
src/pegen/grammar_parser.py Outdated Show resolved Hide resolved
src/pegen/python_generator.py Outdated Show resolved Hide resolved
@0dminnimda
Copy link
Contributor Author

0dminnimda commented Jul 18, 2022

I find even more concerning that no test found the issue

Should I add the tests for it here? Tho seems appropriate for another pr

@0dminnimda
Copy link
Contributor Author

Maybe @pablogsal have other comments?

@pablogsal
Copy link
Contributor

Maybe @pablogsal have other comments?

I will try to review this soon but currently, I am very busy with the 3.11.0b5 release :(

Copy link
Collaborator

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

I left a minor suggestions for improvement and a question otherwise LGTM.

src/pegen/python_generator.py Outdated Show resolved Hide resolved
src/pegen/python_generator.py Show resolved Hide resolved
@0dminnimda
Copy link
Contributor Author

@MatthieuDartiailh can you allow the workflow?

@MatthieuDartiailh
Copy link
Collaborator

Done

@MatthieuDartiailh
Copy link
Collaborator

@pablogsal would you have time to look at this ? or do you want to defer the final review to me maybe (since you seem quite busy ATM).

@pablogsal
Copy link
Contributor

@pablogsal would you have time to look at this ? or do you want to defer the final review to me maybe (since you seem quite busy ATM).

I apologize for the late reply @MatthieuDartiailh. Unfortunately the release of 3.11 is being ridiculously challenging and I'm expending all my OSS time on that :(

Please, go ahead and merge it if you feel confident. I trust your criteria 👍

Co-authored-by: Matthieu Dartiailh <marul@laposte.net>
@MatthieuDartiailh
Copy link
Collaborator

I wanted to add a couple small comments but apparently I cannot push to your repo. S I will merge and add the comment directly on main.

@MatthieuDartiailh MatthieuDartiailh merged commit b224cd0 into we-like-parsers:main Sep 22, 2022
@0dminnimda 0dminnimda deleted the stop-creating-unused-variables branch September 22, 2022 22:12
@0dminnimda
Copy link
Contributor Author

Weird usually there's no problem in that
Maybe i accidentally checked out Allow edits by maintainers..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants