Skip to content

fix(config): Always output JSON format in yarn config list#4708

Closed
sreeramjayan wants to merge 3 commits intoyarnpkg:masterfrom
sreeramjayan:4496
Closed

fix(config): Always output JSON format in yarn config list#4708
sreeramjayan wants to merge 3 commits intoyarnpkg:masterfrom
sreeramjayan:4496

Conversation

@sreeramjayan
Copy link
Copy Markdown

@sreeramjayan sreeramjayan commented Oct 13, 2017

Summary

Fixes #4496.

Used JSON.stringify to ensure the output of yarn config list displays JSON.

Test plan

New integration test to validate JSON output.

@arcanis
Copy link
Copy Markdown
Member

arcanis commented Oct 16, 2017

Can you share a pic of before/after?


reporter.info(reporter.lang('configYarn'));
reporter.inspect(config.registries.yarn.config);
reporter.inspect(JSON.stringify(config.registries.yarn.config, null, 2));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you should use reporter.log at this point with the {force: true} option.

@BYK
Copy link
Copy Markdown
Member

BYK commented Oct 16, 2017

Hey, thanks a lot for the PR!

A test case would be great if you can add it. A simple integration test in __tests__/integration.js would be sufficient to make sure the output is always valid JSON.

Also having a proper title for the PR would be great. We use the following format:

fix(config): Always output JSON format in `yarn config list`

**Summary**

Fixes #4496. Use `JSON.serialize` instead of `reporter.inspect` to ensure the output of the config object is always a valid JSON string.

**Test plan**

Added new integration test that checks JSON output.

@BYK
Copy link
Copy Markdown
Member

BYK commented Oct 17, 2017

@sreeramjayan let me know if you need any help with the tests 😄

@sreeramjayan
Copy link
Copy Markdown
Author

sreeramjayan commented Oct 18, 2017

@BYK I did make the requested changes but am facing an issue coming up with the test case.
sreeramjayan@462afb2

The issue seems to be that the output of running yarn config list isn't JSON since we get the results of the yarn and npm config.
https://github.com/sreeramjayan/yarn/blob/462afb2de70c90a85ba892eaa423c643068ef788/__tests__/integration.js#L420
I was wondering how the output can be validated to be JSON.

I changed the PR's summary and title as well.

@sreeramjayan sreeramjayan changed the title 4496: Fix inconsistent config strings in yarn config list. Oct 18, 2017
@BYK BYK changed the title Fix inconsistent config strings in yarn config list. fix(config): Always output JSON format in yarn config list Oct 26, 2017
@BYK
Copy link
Copy Markdown
Member

BYK commented Oct 26, 2017

Sorry for the late response and thanks a lot for the updates!

The issue seems to be that the output of running yarn config list isn't JSON since we get the results of the yarn and npm config.

I think you can get a "pure" JSON string if you run yarn --silent config list ;)

Copy link
Copy Markdown
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

I would also like a screenshot :)

The main interest of inspect is that it comes with colors, so I believe that using a basic JSON serializer will actually add a bug.

@sreeramjayan
Copy link
Copy Markdown
Author

sreeramjayan commented Oct 31, 2017

@arcanis Here are the before and after screen shots.
Before:
before

After:
after

As you mentioned the formatting is off when the string is serialized. Any ideas on what can be done regarding this? Since the some of the keys don't have hyphens they aren't formatted as the other keys are.

@arcanis
Copy link
Copy Markdown
Member

arcanis commented Feb 27, 2018

Hi! I'm really grateful for the work you put in this PR, but unfortunately the project moved forward since you submitted it and I fear it can be merged in this current state. I'm going to close it, but please feel free to check if the problem still exists, and to rebase your work on top of the current master and open a new PR if it's the case. Again, sorry we didn't got around merging it sooner!

@arcanis arcanis closed this Feb 27, 2018
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.

3 participants