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

Sort requires #10616

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Sort requires #10616

merged 1 commit into from
Jan 11, 2017

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Jan 4, 2017

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Jan 4, 2017
@sam-github
Copy link
Contributor Author

@gibfahn re: #10389 (comment)

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Trott
Trott previously requested changes Jan 4, 2017
Copy link
Member

@Trott Trott left a 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.

@sam-github
Copy link
Contributor Author

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.

@sam-github
Copy link
Contributor Author

@Trott worth it, or should I close?

@gibfahn
Copy link
Member

gibfahn commented Jan 10, 2017

@sam-github I wonder if there's an easy way to add a linter rule for this?

@Trott
Copy link
Member

Trott commented Jan 10, 2017

I personally don't lexically sort them. I put common first (which is required), then usually assert next, then whatever module is being tested, and then whatever modules are needed additionally (often path or url or fs or...).

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: :-|

@Trott Trott dismissed their stale review January 10, 2017 23:16

changes requested have been made; neutral on the change overall

@Trott
Copy link
Member

Trott commented Jan 10, 2017

(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.)

@sam-github
Copy link
Contributor Author

@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>
@sam-github sam-github merged commit 02b9270 into nodejs:master Jan 11, 2017
@sam-github sam-github deleted the sort-requires branch January 11, 2017 17:06
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
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>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
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>
MylesBorins pushed a commit that referenced this pull request Mar 7, 2017
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>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants