search

we-like-parsers / pegen

PEG parser generator for Python
https://we-like-parsers.github.io/pegen/
MIT License
150 stars 32 forks source link

Stop creating unused variables #68

Closed 0dminnimda closed 1 year ago

0dminnimda commented 2 years ago

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

pablogsal commented 2 years ago

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

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

0dminnimda commented 2 years ago

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

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

0dminnimda commented 2 years ago

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

0dminnimda commented 2 years ago

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

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

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

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

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

0dminnimda commented 2 years ago

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

gvanrossum commented 2 years ago

Bye, I am leaving this repo now.

0dminnimda commented 2 years ago

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

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

0dminnimda commented 2 years ago

Closing and opening to triger the ci

0dminnimda commented 2 years ago

oh yeah, forgot to merge

0dminnimda commented 2 years ago

Is there any additional comments or this can be merged?

0dminnimda commented 2 years ago

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

Maybe @pablogsal have other comments?

pablogsal commented 2 years ago

Maybe @pablogsal have other comments?

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

0dminnimda commented 1 year ago

@MatthieuDartiailh can you allow the workflow?

MatthieuDartiailh commented 1 year ago

Done

MatthieuDartiailh commented 1 year ago

@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 commented 1 year ago

@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 :+1:

MatthieuDartiailh commented 1 year ago

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.

0dminnimda commented 1 year ago

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