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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to change detectIndent option #16

Open
vladshcherbin opened this issue Oct 20, 2019 · 7 comments
Open

Add possibility to change detectIndent option #16

vladshcherbin opened this issue Oct 20, 2019 · 7 comments

Comments

@vladshcherbin
Copy link

Hey 馃憢

Internal write-json-file has a detectIndent option to detect indentation of existing file which is false by default. write-pkg provides this option which is hardcoded to true.

If existing file has empty {} contents inside, added fields will be saved as a single line:

{"name":"test","version":"1.0.0"}

It's impossible to fix such file indentation using any indentation options, the only way is to remove file and regenerate it. It would be nice to have detectIndent not hardcoded and have possibility to overwrite it.

@sindresorhus
Copy link
Owner

I think it would be better to fix your specific issue instead. We could just default to 2-space indentation if the existing indentation could not be detected.

@vladshcherbin
Copy link
Author

vladshcherbin commented Oct 24, 2019

@sindresorhus can you give more details? Do you mean check indentation using detect-indent and then pass other options in this package or something else?

@vladshcherbin
Copy link
Author

Actually, I do think its better to fix this using the linked PR. I can give you another use case - you can have badly indented package.json and want to fix it using indent: 2 for example. It also won't work because detectIndent is set to true.

There may be another use cases, I do think it's a better idea to give control and choice to a developer.

I've changed the issue name, maybe it gave false meaning, the PR doesn't remove default true option, it just gives you a way to change it if needed.

@vladshcherbin vladshcherbin changed the title Remove hardcoded detectIndent option Add possibility to change detectIndent option Oct 24, 2019
@sindresorhus
Copy link
Owner

Actually, I do think its better to fix this using the linked PR.

The linked PR is not fixing it. It's giving you the ability to work around it. That's different. I want to actually fix it so everyone doesn't have to specify the option to get the expected behavior when the package.json is empty.

you can have badly indented package.json and want to fix it using indent: 2 for example. It also won't work because detectIndent is set to true.

I'm not a big fan of doing changes based on imaginary scenarios. There's also no indent option here.

@vladshcherbin
Copy link
Author

The linked PR is not fixing it. It's giving you the ability to work around it.

Oh, I see what you mean, totally understandable :)

I'm not a big fan of doing changes based on imaginary scenarios. There's also no indent option here.

I'm using indent option from write-json-file, which sets indentation to 2. My use case is:

No matter what indentation is in existing file, set indentation to 2 spaces. It works with detectIndent: false + indent: 2 combo. So, the first issue is with {}, second is with non-updatable indentation.

I see by default you set detectIndent to true to respect existing indentation, however I don't need to use existing indentation and my use case won't work if detectIndent is true.

Suggest how to solve this to work both ways, I'll update PR :)

@vladshcherbin
Copy link
Author

So, I see a fix in two parts:

  1. Solve issue with {} for everyone with 2 space default indentation.
  2. Give possibility to set detectIndent to false for cases like mine.

@sindresorhus what do you think?

@sindresorhus
Copy link
Owner

Sorry, I totally missed your comment. Yes, that plan sounds good.

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

Successfully merging a pull request may close this issue.

2 participants