-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Sort requires #10616
Sort requires #10616
Conversation
2cdec46
to
0192087
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs the corresponding change on lines 86 and 87.
0192087
to
da3617b
Compare
Was sorted in another PR. I still have an added line in the description pointing out that the sorting wasn't accidental that I can land if wanted. |
@Trott worth it, or should I close? |
@sam-github I wonder if there's an easy way to add a linter rule for this? |
I personally don't lexically sort them. I put I don't oppose lexically sorting them. (Maybe we should call it "alphabetical sorting" as that may be a term more people will be familiar with than "lexical sorting"?) I'm not really for it either. Totally neutral. Happy to do it if there's consensus that is the way it ought to be done. Happy to not dictate it if that's the consensus. Here, have a neutral face emoticon: :-| |
changes requested have been made; neutral on the change overall
(Given that there are five approvals for this change and no one saying "no no no", I think you can safely land this. If someone dislikes it, they can say so after the fact. You've had it open for 6 days already. I say this as someone who doesn't actually have an opinion as to whether or not it should land. I guess I like the sound of myself typing.) |
@Trott I used to sort my includes like that, based on some kind of semantic grouping, and I liked it. But when it comes to trying to describe to people my reasoning so they can unthinkingly reproduce it, I eventually realized its just too complex to document, and ends up not being reproduceable, you get essentially random placement. And maybe that's OK, but as the require list gets longer, it gets harder to find (visually) a require if you are looking for it. Luckily, eslint warns about duplicates, now, so its less common to find a require added multiple times. Still, I now sort everything that can be reasonably sorted. The ref links at the bottom of doc/api/markdown files drive me batty. Its just a random soup down there, so I'm sorting them, too, as I get a chance. I'll be more precise about the sorting criteria, then land it. |
PR-URL: nodejs#10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
da3617b
to
02b9270
Compare
PR-URL: nodejs#10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: nodejs#10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: nodejs#10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: nodejs#10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: #10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: #10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: #10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
PR-URL: #10616 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Checklist
Affected core subsystem(s)
doc