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

Docs: Updated Working with Custom Formatters (fixes #9950) #10592

Merged
merged 3 commits into from
Jul 14, 2018
Merged

Docs: Updated Working with Custom Formatters (fixes #9950) #10592

merged 3 commits into from
Jul 14, 2018

Conversation

marla294
Copy link
Contributor

@marla294 marla294 commented Jul 10, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ X ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Rewrote the Using a JSON formatter first section of documentation.

  • Removed example using stdin and stdout
  • Added example showing how you could use ESLint's built-in JSON formatter and pipe the output to another program that accepts JSON as input.
  • Added link to ESLint's built-in formatter page.

Is there anything you'd like reviewers to focus on?
Let me know if there's anything you'd like to see different with this, and I'll update. Thanks for allowing me to contribute!

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 10, 2018
@kaicataldo kaicataldo added bug ESLint is working incorrectly documentation Relates to ESLint's documentation accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 10, 2018
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to ESLint! One small question for clarity.

@@ -254,48 +296,13 @@ error semi

### Using a JSON formatter first
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can make this header clearer now? Something that gets across Outputting to a standard format for other programs to consume? I don't like that exactly, but does that make sense?

```

And then the formatter can read from stdin
You can use ESLint's built-in JSON formatter first, and pipe the output to another program that accepts JSON linter results.
Copy link
Member

Choose a reason for hiding this comment

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

If we change the header of this section this could be You can use ESLint's built-in JSON formatter and pipe the output to another program that accepts JSON linter results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! I hear you on the title being kinda off now. I'll think about alternate titles for the section and get back to you, probably tomorrow. Will also make that change in wording once the title is nailed down. Thanks for reviewing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, how about just "Using built-in formatters with custom formatters" ? I guess you could use the other formatters too, like HTML or something, and then send it to another program to do something with. Since this is the "using custom formatters" page, the "other program" is probably going to format the results. LMK what you think or if you've got other ideas. I'll hold off on submitting another PR until we come to a decision.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a bit confusing, personally. I wonder if Outputting to a standard format for other programs to consume actually does work? I think it gets across what this section is trying to convey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, okay. It's a little long but it's probably better to be too long than be confusing. How about we try it out? I'll change the title to that and resubmit the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good - we can always change it later if it's confusing! No need to resubmit - you can either add another commit to this branch (we squash and merge) or amend the current commit and force push it. Thanks for working on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! I'll try to amend the current commit. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to amend the current commit, it looks like it made a new commit. Is there anything else I need to do, or will this just get squashed and merged? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Multiple commits is fine! We'll just squash when we merge. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! Thank you!

Updated title of last section to "Outputting to a standard format for other programs to consume"
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM. Leaving this open for another day or two so that others can take a look. Thanks for contributing to ESLint!

@kaicataldo
Copy link
Member

Ah, one final thing from me: do you mind updating the title of the PR to the first commit message (Docs: Updated Working with Custom Formatters (fixes #9950))? I think it's a little more descriptive.

@marla294 marla294 changed the title Docs: Addresses Last Issue (fixes #9950) Docs: Updated Working with Custom Formatters (fixes #9950) Jul 11, 2018
@marla294
Copy link
Contributor Author

Sure thing - updated!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@platinumazure platinumazure merged commit 9f93d5f into eslint:master Jul 14, 2018
@platinumazure
Copy link
Member

Merged! Thanks @marla294 for improving the documentation!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 11, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants