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

Support singular 'script' dirname convention #72

Merged
merged 4 commits into from
Dec 17, 2018

Conversation

jasonkarns
Copy link
Member

@jasonkarns jasonkarns commented Dec 17, 2018

I can't find any documentation or any source that finds either of 'scripts' or 'script' to be more common in practice.

Absent any data, I lean only on anecdotal experience when I find 'script' to be more common in the wild than 'scripts'.

To further defend this, GitHub has blogged their "Scripts to Rule Them All" pattern, and for it they use 'script' singular.

This change adds 'script' (and 'script-win') as fallbacks if the plural version does not exist.

This should be a semver minor change.

Any users who may incidentally already have both scripts and script directories, scripts will continue to be preferred (as long as it exists). The singular 'script' is only used as a fallback when scripts does not exist.

The same is true for Windows users. scripts-win is preferred if both scripts-win and script-win exist.

The only users who may experience an issue are Windows users who do not have a scripts-win directory but do have a script-win directory, for some reason. In this case, what would have fallen back to scripts will now be preempted by script-win.

closes #52

I can't find any documentation or any source that finds either of
'scripts' or 'script' to be more common in practice.

Absent any data, I lean only on anecdotal experience when I find
'script' to be more common in the wild than 'scripts'.

To further defend this, GitHub has [blogged](https://githubengineering.com/scripts-to-rule-them-all/)
their "Scripts to Rule Them All" pattern,
and for it they use 'script' singular.

This change adds 'script' (and 'script-win') as fallbacks if the plural
version does not exist.

This _should_ be a semver minor change.

Any users who may incidentally already have both scripts and script
directories, `scripts` will continue to be preferred (as long as it
exists). The singular 'script' is only used as a fallback when `scripts`
does _not_ exist.

The same is true for Windows users. `scripts-win` is preferred if both
`scripts-win` and `script-win` exist.

The _only_ users who may experience an issue are Windows users who do
not have a `scripts-win` directory but do have a `script-win` directory,
for some reason. In this case, what _would_ have fallen back to
`scripts` will now be preempted by `script-win`.
@jasonkarns jasonkarns force-pushed the scripts-to-rule-them-all branch from a16a1c2 to 753ac33 Compare December 17, 2018 17:22
These directories were all empty so they were implicitly ignored by git.
Include any empty .gitignore file so they'll be explicitly tracked.
@searls
Copy link
Member

searls commented Dec 17, 2018

seems legit

Refactors script-dirs tests to stub fs.existsSync instead of stubbing
process.cwd. (This removes the need to have fixtures of the various
script directory combinations.)

Additionally, this adds a test asserting that module script lookup
supports both scripts and script. (The latter being the default if
the former doesn't exist.)
@jasonkarns jasonkarns force-pushed the scripts-to-rule-them-all branch from aaf126e to f5e55b4 Compare December 17, 2018 19:28
@jasonkarns
Copy link
Member Author

@searls latest commit refactors the tests to stub fs.existsSync instead of stubbing process.cwd(). The latter required there being fixture directories such that the existsSync calls would pass/fail accordingly. I think the stubbing of existsSync is more clear since the tests are now explicit about directories existing or not (as opposed to that knowledge living in the fixtures).

Also adds a test to cover the scripts -> script fallback behavior for modules as well.

I've removed the Do Not Merge flag, so it's merge-ready if tests/code are 👍

@searls searls merged commit dd29524 into master Dec 17, 2018
@searls
Copy link
Member

searls commented Dec 17, 2018

Skimmed and merged

@searls searls deleted the scripts-to-rule-them-all branch December 17, 2018 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support script/ along with scripts/ by default
2 participants