Rename *.l files to *.re #4172
Rename *.l files to *.re #4172
Conversation
This syncs PHP lexer files to all use *.re extension. The *.re files are processed with the RE2C tool.
|
No. All these extensions are made up. The re2c docs uses So the |
|
Applied via 9690477 to PHP-7.4+... |
|
👎 |
|
@nikic explain more? |
|
There wasn't really justification for this change: As you said the
extensions are made up, there is no standard to be inconsistent with.
What we've gained by making this change is not obvious to me, but you did
just break every internals tutorial that touches the lexer, you broke
information in the only docs we have for internals on phpinternalsbook, you
broke every patch anyone is working on that touches the lexer (or any
changed files).
Please be more careful when making this sort of change, and especially
don't be in such a rush to make such changes, always leave enough time to
get feedback, preferably from more than one person.
…On Sat, 18 May 2019, 21:57 Peter Kokot, ***@***.***> wrote:
@nikic <https://github.com/nikic> explain more?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#4172?email_source=notifications&email_token=AARB52SFBMLNU4ISCHYQG6TPWBNTFA5CNFSM4HNRROT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVWU7VI#issuecomment-493703125>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARB52V6FIDIA6KSKMNL5VLPWBNTFANCNFSM4HNRROTQ>
.
|
That was what I had in mind :) I don't really care about any of the rest, but zend_language_scanner.l has been a fixture for a long time. Not to mention that history for zend_language_scanner.re is now lost, or needs to be accessed in very roundabout ways (and I don't think we'll recover it even if you revert the rename, though it would be worth checking in a fork). Thankfully git blame at least still works. |
|
Thank you both for the explanation (this then must be a common thingy here if there are two with this issue). I guess we need to have some disagreements here and there. Let's go through this one more thoroughly then... Because in my opinion it is a problem of who did this (a random newbie coming to contribute to PHP unlike some internals guy present there for ages and being more known for their contributions and all) and not the change itself. Bottom line, the wrong person committed this. And I accept this. No problem :) And I'm completely open for the opinions if they are justified. Theoretically, if we add ext/foo with php_foo_lexer.x there that re2c software will generate the belonging Besides, with the PHP-7.4 the generate lexer files have changed. They aren't tracked in the Git repository anymore. And this is quite a change actually. Because building PHP differs... Yes, the Git history works just fine with that Git blame. There isn't any history in the GitHub interface of all the listed contributors on the top of the page that is true, and I know that and I always also check if the rename is ok (btw, the rename back returns that history - also in Git). That is a problem of PHP credits pages and how to recognize contributions and all. Scrolling through the history of the previous file is so simple, that you will have a very hard time convincing me that this can justify not moving forward with these particular file renames. The consistency out weights this problem completely. For the mentioned tutorials please send the links so I can send patches there. I haven't found any occurrences in the phpinternalsbook.com for these 3 (three) files. I must be checking some other repo then. Apologies if this small not important change caused you some issues but I really don't have 14 days to wait for a confirmation on something so minimal. Yes, the feature freeze is coming up for the PHP-7.4 release and tend to respect that hoping I won't need to fix too many things that can't go to the changelogs. Kind regards and wish you both a pleasant day forward. |
|
P.S: I have now also checked the doc-en (official PHP manual) and I have found zero occurrences of these files. So all cases solved. Other documentations are a bit out of my scope here. ;) |
Not really the wrong person committing, but the wrong person approving. For changes to Zend it's good to have an engine dev have a look. The thing with these kinds of changes is that they have very marginal value -- but of course the cost of them is usually equally marginal, which is why I'm usually fine with doing them and have approved many PRs for these kind of renames/moves/etc. This is just a case where the cost/benefit ratio imho falls on the wrong side of the line.
It might look like a more significant change, but this actually had absolutely no impact on core dev. Impact is not always a function of lines of code changed :)
The list of contributors isn't important. The problem is that https://github.com/php/php-src/commits/master/Zend/zend_language_scanner.re is now empty. And getting to the old history takes a lot of steps (go to the rename commit, go to the parent commit, change to tree view, find the correct file, open it, go to history). For future reference, here is the missing history: https://github.com/php/php-src/commits/63ef554fe48a042088066fc97a7422cbfe983205/Zend/zend_language_scanner.l
https://phpinternals.net/articles/implementing_a_digit_separator and similar blog posts (there are a few more on the same site) |
I'd like to apologize here for my approval without been an engine dev. Maybe (not trying to make the process of contribution to |
|
If reverting the change restores history, then please do it.
It's not the case that we can patch our way out of the problem, we can't
patch books on bookshelves or people's personal blogs.
It is necessary to wait for proper approval sometimes, if you don't have
the time to wait for that approval then don't propose the change, there is
no other option.
…On Sat, 18 May 2019, 23:36 Gabriel Caruso, ***@***.***> wrote:
Not really the wrong person committing, but the wrong person approving.
For changes to Zend it's good to have an engine dev have a look.
I'd live to apologize here. Maybe (not trying to make the process of
contribution to php-src, but avoid situations like this) we can use the
GitHub functionality called code owners
<https://help.github.com/en/articles/about-code-owners>, to enforce that
any change in the Zend/ folder should have at least one approval from an
Engine Dev.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4172?email_source=notifications&email_token=AARB52SI2YCQPQH7FOCWWATPWBZGPA5CNFSM4HNRROT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVWWNCY#issuecomment-493708939>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARB52WY67JGQWFWIKMOZMLPWBZGPANCNFSM4HNRROTQ>
.
|
|
So, let me check this again if I understand this correctly. This particular logical and consistency change is a problem due to 4 reasons now:
The command There is also browser extension that can do that now. Has just been patches few weeks ago.
Ok... Great, nice that I understand the for the record written scope of this issue. For the off the record I fully understand the problem here. Good day. |
No pointing fingers or anything, but rushing a change is also not a good thing, that goes to every change proposed, big or small. Like pointed out above, getting a confirmation from someone who works on the related piece of the code by pinging them if they don't pick up on it themselves. Most of us volunteer to the project, absence, travels and other related things happen, it is just a natural flow of a FOSS project. This however is not a critical change so there is no need to push it in (just using this as an example), this is not a blocker for a feature freeze and could have happened in a beta release too even. In regards to approvals, I did ask @nikic a while back about limiting approvals, especially since we have some users on Github that randomly approves PRs, whether or not its because they want a feature in or not, but we should limit that to a team of the @php repository. In the same subject, we should strive to not approve something we are not familiar with. For example, I haven't worked with On the topic; like mentioned by @krakjoe, if reverting this brings back the history, then lets do that and learn from this lesson and move forward towards |
|
Can we also then please sum up an official response for the documentations out there what is the logics of having Thanks. When this is done, I'll be in favour of reverting that. |
When feature freeze happens changes like this are not exactly welcome in the commit history. Or let's say the least the better. This approach has happened many times actually so far that PHP release manager didn't want to merge my changes in that branch. So, I have learned from that practices that feature freeze means also the state of having a typo in the comment (theoretically). If it can't be logged in the NEWS it shouldn't go in the branch anymore. Similar as the fixing small things in the PHP-7.3 branch now - I have a very long list of what should be fixed there but I can't do it now anymore and things go to PHP-7.4 branch. Yes, we have done some changes for practical reasons like whitespaces fixes, like enabling mysql finally on the Appveyor and travis (!?) etc that need to be done on previous branches also. But these are at least trying to be minimalistic as much as possible. So, yes, I have only until the feature freeze to fix some of these things... :) |
|
I also strongly suggest to start integrating the so called CODEOWNERS file (configuration-alike file for the list of repo locations and github handles that get request for a reviews): We (sorry, I mean you - the PHP engine devs) just define the people (GitHub handles) that should get notified when some pull request targets their repository location and asks them for a review. And you won't have to deal with the rest of "us" reviewers bystanders anymore. Case elegantly solved. I can start patching and wrapping up a pull request for this... Let me know. |
There's no official response, there are the preferences of the developers working on those changed files: we prefer not to break history and content in the absence of a good reason to do so.
Sorry, but this is not fair. If we are having a bad day its not because anyone committed anything, but because of the response to our saying we are not pleased with that commit. You didn't do anything wrong when you made the commit: you didn't know about or consider history, you didn't know who should approve the change, you didn't know about content it might break ... that's all totally fair. The way you are responding is not fair, we have requested that the merge is reverted and given our reasoning for that. We should not be having to argue the case further, you made a mistake and you need to revert it, please do so, or I'll do it myself. |
|
We will now have some *.re files and some *.l files because of your ego? |
|
I'm not sure I understand any of the logics presented here to do the revert and frankly I never ever will. You all know that what you're explaining here fights against your logics also. Really bad that we cannot improve the files structure in such ways... Despite all the given reasons I gave.... For the history of the files and for the written books and tutorials out there. OMG. I'll move to other pull requests and bugs for a while because this is such a nonsense... On the record, this entire pull request is now the official explanation also. And a very bad one because you can't understand that opening the repository having gazillions of different file naming strategies is really hard to grasp from user point of view and the editor's / IDEs... |
|
@carusogabriel No need to apologize, you didn't do anything wrong here. My comment was inappropriate here -- I don't think this PR can be classified as an "engine change" under any reasonable interpretation of the term, and reviewing this does not need any engine expertise. It's just a case where we happen to disagree :) @petk I think the crux here might be that "despite all the given reasons I gave" didn't really make it through to me -- you posted refutations for our points, but I didn't see any justification of the change in the first place, beyond a general aesthetic goal. Who benefits from this change? Engine developers? Certainly not. Newbies? I don't think so. For them the extensions I think that new contributors would get much more mileage out of adding a comment to the top of zend_language_scanner along these lines, rather than a rename:
I'm not very bothered by this change in itself, but what does bother me is that I'm apparently not allowed to express my disagreement with a change anymore. I'm sorry that I initially only posted the thumbs down (which was certainly rude), but I think that after that both @krakjoe and me tried to express the technical concerns we had, and chalking this up to anyone's ego does not move this discussion in any productive direction. You have been making somewhat passive-aggressive comments on PRs and the mailing list about your changes changes being rejected, so I suspect that this specific PR just become a proxy for more general concerns you have. I approve pretty much all PRs you submit (sometimes after minor comments), so I'm not really sure where this is coming from. If you want to discuss that, feel free here or via mail... |
|
Same goes for me, if there is something you wish to discuss then my email is always open too and I'm certain most other long time active project members too are available |
|
Hello, thanks for additional explanation. Then apologies if that sounded like that. Yes, I know it is a must do sometimes because a lot of time goes to waste here. I'll distant myself here a bit from this from now. @nikic @krakjoe @KalleZ you can revert the change if you think that having file commit history on a single click (instead of two) on GitHub + tutorials out there with existing *.l file names is more important than the same file types group. The revert is very simple: git checkout PHP-7.4
git pull --rebase
git revert 969047749d33bb88a0573aa91a57e2070335111
git checkout master
git merge --no-ff --log PHP-7.4
git push origin master PHP-7.4And voila. I'll also propose other adjustments regarding this a bit later because I think it is enough for one release for now (expect more on these stuff somewhere in the beginning of 2020, for PHP 8 - when appropriate). No prob, carry on... I've accepted also this then. I'll go to bugs phase now. Regards. |
This syncs PHP lexer files to all use *.re extension. The *.re files are processed with the RE2C tool.