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

Make --pretty-json work with Standard JSON output #11625

Closed
wants to merge 0 commits into from
Closed

Make --pretty-json work with Standard JSON output #11625

wants to merge 0 commits into from

Conversation

sladecek
Copy link
Contributor

@sladecek sladecek commented Jul 6, 2021

My attempt to solve #11583. The command-line option '--pretty-json' works both for '--standard-json' and '--combined-json'. There is a new option '--pretty-json-indent' which defines indentation level with default value 2.

Fixes #11583

@cameel
Copy link
Member

cameel commented Jul 6, 2021

Great! Thanks for the PR! I'll do a proper review tomorrow but here's some general feedback for now:

  • Could you rebase your code on CommandLineParser #11518? That PR is going to be merged soon and it will conflict with yours. It moves a lot of stuff from CommandLineInterface to a separate class and adds tests.
  • Not sure about the new option. Why not just do --pretty-json=N? boost::program_options can handle that. You can make the value optional and only assign to it if the option is specified.
  • Please use uint32_t or uint8_t or a similar type with platform-independent size rather than unsigned. Otherwise external tools don't have a clear range of values to use for validations (this came up for example in Not possible to set max value of config.solidity.settings.optimizer.runs NomicFoundation/hardhat#1589). We currently do use unsigned for another parameter (--runs) but we should really change that too.

@cameel
Copy link
Member

cameel commented Jul 6, 2021

Also, your PR looks like a complete solution for #11583 so you should put Fixes #11583 in your first comment. This will close that issue automatically once we merge it.

I'd also leave the issue number out of the PR title. I don't think it helps that much. We don't know all these numbers by heart anyway :) Putting it in the description is enough. Same goes for commit description - it spams the issue with a lot of useless entries when rebasing so we do not do that (please see how it looks on the issue page now :)).

@sladecek
Copy link
Contributor Author

sladecek commented Jul 6, 2021

Thank you for the quick feedback. I'll try to address all the issues. The new option was added to allow adding more pretty print parameters other than indent.

@sladecek sladecek changed the title Make --pretty-json work with Standard JSON output #11583 Make --pretty-json work with Standard JSON output Jul 6, 2021
@sladecek sladecek closed this Jul 7, 2021
@cameel
Copy link
Member

cameel commented Jul 7, 2021

@sladecek I see you closed the PR. It looks like it might have been closed automatically by github because of a rebase. If that's the case, please reopen or just open a new one.

@sladecek
Copy link
Contributor Author

sladecek commented Jul 7, 2021 via email

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.

Make --pretty-json work with Standard JSON output
2 participants