Skip to content

Conversation

@zertosh
Copy link
Contributor

exts and trailingSlash are only used if the path isn't cached.

@Trott
Copy link
Member

@Trott
Copy link
Member

The first line of the commit message should start with lib: rather than module:. No big deal if you don't fix it as whoever lands this can fix it. But if you can change it, that will save a few keystrokes for whoever lands it.

@zertoshzertosh changed the title module: move unnecessary work for early returnlib: move unnecessary work for early returnOct 29, 2015
@zertosh
Copy link
ContributorAuthor

@Trott done!

@mscdex
Copy link
Contributor

@Trott Why should it start with lib: instead of module:? It seems like the latter would be a more descriptive subsystem target?

@Trott
Copy link
Member

@mscdex I always thought (wrongly, it seems!) that we standardize on the nice short directory names in the top-level directory as much as possible. So lib, src, test, etc. module seemed weirdly long and specific to me . But looking through the commit log, there's plenty of precedence for it. So I retract!

@zertosh Sorry for the bad suggestion. lib: is fine but module: is better.

@zertoshzertosh changed the title lib: move unnecessary work for early returnmodule: move unnecessary work for early returnOct 29, 2015
@zertosh
Copy link
ContributorAuthor

no worries @Trott

@TrottTrott added the module Issues and PRs related to the module subsystem. label Oct 29, 2015
@jbergstroem
Copy link
Member

Afaik -- prefixes usually stick to the filenames in lib/ if that's where most of the change is made.

@jasnell
Copy link
Member

@nodejs/ctc

@trevnorris
Copy link
Contributor

Mostly what @jbergstroem. The name should be the most closely related subsystem you pass to either require() or process.binding(). There are a few exceptions like src: (for general cleanup).

@rvagg
Copy link
Member

If it's not obvious, just pick one, module:, src: or node: will do, it only really matter where you're doing work specific to a subsystem or are clearly inside a subsystem, otherwise the subsystem prefix is almost useless so it's just a matter of coming up with an arbitrary grouping. Let's not bikeshed this ..

@zertosh
Copy link
ContributorAuthor

Seems like everyone is Ok on this- even the commit prefix. Merge?

EDIT: Meant this for #3578

@trevnorris
Copy link
Contributor

LGTM.

@jasnell
Copy link
Member

LGTM

@MylesBorins
Copy link
Contributor

@jasnell@trevnorris should this be merged?

@cjihrig
Copy link
Contributor

LGTM. We could probably change the vars to const or let. And, we should get a fresh CI run.

lib/module.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it, the parenthesis on this line are unnecessary.

@jasnell
Copy link
Member

`exts` and `trailingSlash` are only used if the path isn't cached.
@zertosh
Copy link
ContributorAuthor

@cjihrig: I've rebased on top of master and switched the var to const.

@cjihrig
Copy link
Contributor

Thanks. Last CI was all green, but running one more time just to make sure switching to const didn't break anything. https://ci.nodejs.org/job/node-test-pull-request/1164/

@Trott
Copy link
Member

Trott commented Jan 8, 2016

Ci is green green green.

@targos
Copy link
Member

LGTM

cjihrig pushed a commit that referenced this pull request Jan 11, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: #3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@cjihrig
Copy link
Contributor

Landed in 1285671. Thanks!

@cjihrigcjihrig closed this Jan 11, 2016
MylesBorins pushed a commit that referenced this pull request Jan 11, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: #3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jan 11, 2016
MylesBorins pushed a commit that referenced this pull request Jan 11, 2016
Notable Changes: * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622. - (Mistakenly missing from v5.4.0) - module: move unnecessary work for early return (Andres Suarez) #3579 * Various doc fixes * Various test improvements PR-URL: #4626
MylesBorins pushed a commit that referenced this pull request Jan 12, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: #3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 12, 2016
Notable Changes: * Minor performance improvements: - module: move unnecessary work for early return (Andres Suarez) #3579 * Various bug fixes * Various doc fixes * Various test improvements PR-URL: #4626
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
Notable Changes: * Minor performance improvements: - module: move unnecessary work for early return (Andres Suarez) #3579 * Various bug fixes * Various doc fixes * Various test improvements PR-URL: #4626 Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: #3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: #3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: nodejs#3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: nodejs#3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: nodejs#3579 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Notable Changes: * Minor performance improvements: - module: move unnecessary work for early return (Andres Suarez) nodejs#3579 * Various bug fixes * Various doc fixes * Various test improvements PR-URL: nodejs#4626 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

moduleIssues and PRs related to the module subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@zertosh@Trott@mscdex@jbergstroem@jasnell@trevnorris@rvagg@MylesBorins@cjihrig@targos@Fishrock123