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 parseOpts() option #135

Merged
merged 2 commits into from
Jan 13, 2017

Conversation

novemberborn
Copy link
Contributor

This replaces find-root and pkg-config by pkg-conf.

pkg-conf can find config without throwing exceptions. It returns an
empty object by default, simplifying logic. And, usefully, it lets you
determine the path of the loaded package.json file.

pkg-config caches the JSON, though not the file resolution. I doubt the
caching has any measurable impact when it comes to standard-engine and
its use. Caching could be implemented on top of pkg-conf if necessary
though.

Adds support for a parseOpts() option in options.js. Fixes #132.

novemberborn added a commit to novemberborn/standard-engine that referenced this pull request Oct 31, 2016
`standard-engine` has all the information it needs to resolve these
options. `deglob` shouldn't have to repeat the work of finding the
`package.json`.

This will make it easier to control the `ignore` globs in a custom
options parser, for which see
<standard#135>.

Requires standard/deglob#6 to be released and the
minimum version to be bumped.
@feross
Copy link
Member

feross commented Nov 23, 2016

And, usefully, it lets you determine the path of the loaded package.json file.

This doesn't seem very useful to me. What's the use case?

if (self.customParseOpts) {
var filepath = pkgConf.filepath(packageOpts)
var rootDir = filepath ? path.dirname(filepath) : opts.cwd
opts = self.customParseOpts(opts, packageOpts, rootDir) || opts
Copy link
Member

Choose a reason for hiding this comment

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

This is too magical, let's just require them to return opts.

@novemberborn
Copy link
Contributor Author

And, usefully, it lets you determine the path of the loaded package.json file.

This doesn't seem very useful to me. What's the use case?

You only need one (direct) dependency to get the location of the package.json (which is passed to the customParseOpts function), as well as reading the config stanza from the package.json file.

@feross
Copy link
Member

feross commented Nov 25, 2016

@novemberborn Sounds good. Can you fix up the other inline feedback and I'll merge.

@novemberborn
Copy link
Contributor Author

@feross yup, it's on my to do list.

@feross
Copy link
Member

feross commented Dec 27, 2016

@novemberborn Any chance you can finish up this PR? 🙏

@novemberborn
Copy link
Contributor Author

@feross it's pretty high on my OSS to-do list, though I'm traveling at the moment and there's plenty non-computer distractions 😄

pkg-conf can find config without throwing exceptions. It returns an
empty object by default, simplifying logic. And, usefully, it lets you
determine the path of the loaded `package.json` file.

pkg-config caches the JSON, though not the file resolution. I doubt the
caching has any measurable impact when it comes to `standard-engine` and
its use. Caching could be implemented on top of pkg-conf if necessary
though.
@novemberborn
Copy link
Contributor Author

Fixed merge conflict and addressed feedback.

@feross
Copy link
Member

feross commented Jan 13, 2017

Nice, looks good to me now!

@feross feross merged commit fb53ede into standard:master Jan 13, 2017
@novemberborn novemberborn deleted the additional-option-parsing branch January 13, 2017 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support additional option parsing
2 participants