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

fix(putout) make defaultOpts use .putout.json #4

Closed
wants to merge 3 commits into from
Closed

fix(putout) make defaultOpts use .putout.json #4

wants to merge 3 commits into from

Conversation

hntrl
Copy link
Contributor

@hntrl hntrl commented Jun 20, 2019

putout(source, require('../.putout.json')) works great, but could be better.

@coderaiser
Copy link
Owner

Thank you, that's interesting idea :). The only problem is putout should have ability to be used in a browser (fkling/astexplorer#414).

Could you please tell me why do you need this? Maybe we can make things work in another other way. For example, you trying to read current directory only, but cli-version search current directory and up till it finds .putout.json, if can't it search of a package.json.

What we can do, is move out getOptions into lib/getOptions. And add it to putout. So your code will looks like this:

const putout = require('putout');
const {readOptions} = putout;

putout(source, readOptions());

This will help to:

  • simplify cli version,
  • avoid duplication,
  • test this code

What do you think about it?

@hntrl
Copy link
Contributor Author

hntrl commented Jun 21, 2019

This shouldn't conflict with the cli since all we're doing here is setting the default object of options.

The cli works great, i'd just expect that the default options in the API be the .putout.json in the root of the project. If not there then it should at least be documented here. ( had some trouble the first time I was using the api (: )

I just pushed some more changes that should add browser support.

@coderaiser
Copy link
Owner

Look, this breaks tests, duplicate logic, and makes harder to support the core engine :). What I can suggest you is such API, if you need this:

const putout = require('putout');
const {readOptions} = putout;

putout(source, readOptions());

When putout used as a library it shouldn't read any configuration files without direct call. This is what readOptions can provide, and what we can add to documentation.

This improves quality of a code base, adds ability to test readOptions function in a normal way and simplifies cli engine.

Your solution is a pure duplication of getOptions implemented in cli which is not handle all the cases.

@hntrl
Copy link
Contributor Author

hntrl commented Jun 25, 2019

ah, I see. Looks like I was just misunderstanding you before. Sorry about that! 😅

@hntrl hntrl closed this Jun 25, 2019
@hntrl hntrl deleted the patch-1 branch June 25, 2019 03:20
@coderaiser
Copy link
Owner

Could you please tell me how do you use putout? Maybe what you need is some kind of defaultOptions with all rules enabled by default similar to the way cli version works?

It can be something like:

const putout = require('putout');
const {getDefaultOptions} = putout;

putout(source, getDefaultOptions());

So you wan't need .putout.json. What kind of rules are you disable?

@hntrl
Copy link
Contributor Author

hntrl commented Jun 28, 2019

I use putout to remove code that isn't relevant to a build. I take out a function using regex, then loop putout until all the unused variables are removed. I don't necessarily need everything else, just the remove-unused-vars rule.

The only reason for this PR was when I was trying to implement with the API, I was confused why the results were always blank. It seems to be documented now so all is well.

Sidenote: it would make sense to have .putout always in the root of the project. .babelrc rollup.config.js .eslintrc.json all reside there, so why shouldn't putout? If I have a putout file in some other directory, i'd just provide an arg. (-c /path/to/.putout.json)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants