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

Add JSON output option to the "config home" subcommand #5946

Merged
merged 3 commits into from Oct 28, 2019

Conversation

@theag3nt
Copy link
Contributor

theag3nt commented Oct 19, 2019

Changelog: Feature: Add --json argument to the config home subcommand to output the result to a JSON file.
Docs: conan-io/docs#1464

As the issue referred to making command output more future-proof it can be written to file in JSON format (with the actual path contained in the home attribute).

Tests have been extended to make sure that the expected output is contained in the JSON file created.

Fixes #5943

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 19, 2019

CLA assistant check
All committers have signed the CLA.

@theag3nt

This comment has been minimized.

Copy link
Contributor Author

theag3nt commented Oct 20, 2019

Based on the latest comment (#5943 (comment)) on the issue I changed the behavior to only output a simple string into JSON.

Copy link
Member

memsharded left a comment

Thanks for contributing this PR @theag3nt !

It is good, the implementation is correct. I think in the past we didn't allow JSON formats that didn't start with a list or a dict, but this is valid JSON today, so seems ok for me, but waiting for @lasote feedback.

@memsharded memsharded added this to the 1.20 milestone Oct 20, 2019
Copy link
Contributor

lasote left a comment

It is better if we provide a dict with a key. Otherwise, we will be very limited if we want to expand this json in the future. It happens all the time. For example, what if tomorrow someone wants json output for conan config get? It would be nice to have a common json output for all the config. Probably, to be prepared, we should return: {"home": "whatever"}

@theag3nt

This comment has been minimized.

Copy link
Contributor Author

theag3nt commented Oct 21, 2019

Sure, I'll just revert to the first version I had before @monsdar's comment. :) Not sure that's reusable enough for a future config-wide JSON output, but exactly matches @lasote's request. I even added an assertion helper preparing for a future expansion of the config home --json output, so the test is checking that it contains {"home": "<path>"} as a subset.

You'll probably want to squash this to keep the history clean.

…ring instead of an object"

This reverts commit 07f1299.
@theag3nt theag3nt requested a review from lasote Oct 24, 2019
@lasote lasote merged commit f0febf7 into conan-io:develop Oct 28, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.