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

feat: Add --indent option to CLI tool #559

Merged
merged 4 commits into from
Jul 7, 2024
Merged

Conversation

danielbayley
Copy link
Contributor

@danielbayley danielbayley commented Jun 30, 2024

Add a --indent option, to be used with --json to output 2 space indentation.

See also #479 and #523.

@danielbayley danielbayley mentioned this pull request Jun 30, 2024
Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

I have two concerns with this change:

  1. Could you clarify what's the use case of pretty-printed JSON output? For single-shot manual use, wouldn't the same be served by piping the output to a separate JSON prettifier like jq?

  2. Naming the option as "pretty" and only affecting the JSON output seems rather misleading. Either the option needs to make sense independently of --json, or it needs to be named more specifically.

@danielbayley
Copy link
Contributor Author

  1. Could you clarify what's the use case of pretty-printed JSON output? For single-shot manual use, wouldn't the same be served by piping the output to a separate JSON prettifier like jq?

Non single-shot manual use… Like in package.json (package.yaml) scripts.

Moreover, I want to do a PR for pnpm to replace it’s dependency on js-yaml (which hasn’t had an update for 3+ years) with this library, which is superior, and handles comments much better!

Forcing this to be inflexible, and add an extra dependency just to make JSON output readable, adding friction to adoption for the sake of basically a single line of code—in fact just a single parameter to an existing builtin function—seems silly…


  1. Naming the option as "pretty" and only affecting the JSON output seems rather misleading. Either the option needs to make sense independently of --json, or it needs to be named more specifically.

I had a similar thought, but there is no such thing as --pretty YAML, it already has to be indented…

I think the ideal API would be an optional parameter, maybe to --json, e.g --json 2 or something, but that isn’t really a feature of util.parseArgs. How about --indent? What other name do you think would be better? @eemeli

This is the yaml.js API, for reference.

@eemeli
Copy link
Owner

eemeli commented Jul 1, 2024

  1. Could you clarify what's the use case of pretty-printed JSON output? For single-shot manual use, wouldn't the same be served by piping the output to a separate JSON prettifier like jq?

Non single-shot manual use… Like in package.json (package.yaml) scripts.

Eh, not sure that it really makes sense to prettify output that's meant for machine consumption, as it risks creating JSON files that will be presumed to be the source of truth, rather than intermediate build assets. But I'll grant you that there's probably a valid use case somewhere here.

  1. Naming the option as "pretty" and only affecting the JSON output seems rather misleading. Either the option needs to make sense independently of --json, or it needs to be named more specifically.

I had a similar thought, but there is no such thing as --pretty YAML, it already has to be indented…

I think the ideal API would be an optional parameter, maybe to --json, e.g --json 2 or something, but that isn’t really a feature of util.parseArgs. How about --indent? What other name do you think would be better?

--indent works, esp. as it's already an option for YAML formatting. Switch this PR to that and make it apply to YAML output as well?

@danielbayley
Copy link
Contributor Author

it risks creating JSON files that will be presumed to be the source of truth

Well the YAML files are the source… But a lot of tools only work from JSON. Harder to debug if it has to be minified also…

--indent works, esp. as it's already an option for YAML formatting. Switch this PR to that and make it apply to YAML output as well?

Good idea, will look into it…

@danielbayley danielbayley requested a review from eemeli July 1, 2024 13:15
@danielbayley danielbayley changed the title feat: Add --pretty option to CLI tool feat: Add --indent option to CLI tool Jul 1, 2024
Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Overall the API looks good, see inline for implementation changes.

src/cli.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
tests/cli.ts Show resolved Hide resolved
tests/cli.ts Outdated Show resolved Hide resolved
Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good! Sorry for the delay in getting to this. There was a missing newline from the test source, which I added to get the test to pass.

@eemeli eemeli merged commit e7c81fe into eemeli:main Jul 7, 2024
13 checks passed
@danielbayley
Copy link
Contributor Author

danielbayley commented Jul 7, 2024

Thank you, this looks good! Sorry for the delay in getting to this. There was a missing newline from the test source, which I added to get the test to pass.

No worries, life gets in the way… I know!

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.

2 participants