[v2] Return unformatted reporter with pretty option

[emmabukacek]

Issues

• https://github.com/SimenB/stylint/issues/362
• https://github.com/sertae/stylint-json-reporter/issues/3

Changes

• Rename reporter config key to formatter
• Update formatter take a native formatter name, a third-party module, or path.
• Rename default formatter as pretty
• Set default formatter to this commit:
  • https://github.com/SimenB/stylint/commit/b041916c84322b8073c0a333fbe2b900ff7331ef
• Update README and add tests
• Delete old reporter tests
• Fix package.json coverage include

Additional

• The formatter argument now works so that you can pass in:
  • Native formatters, like pretty or default
  • Third party formatters inside of your package.json
  • Formatters stored in files, like ./local_repo/linter_formatters/...

Code Coverage

[SimenB]

Thank you for the PR, sorry about the slow response!

The change itself is a good start, but could you make the PR for the v2 branch instead? You'll get quite a lot of conflicts, sorry about that.

To answer your questions:

This breaks the default behavior of the formatting in favor of the old format.

That's fine

Leaving in the 'pretty' reporter seems like a good idea in case some editor parsers updated their code to match. If not, I can strip all that out and we can just revert the commit.

That's also fine, but if you could make --pretty somehow set the reporter option instead of being it's own config inside of stylint, that would be nice 😄 If it's unlcear what I mean, don't hesitate to ask.

If it needs to go into a separate file altogether, I can do that.

Yes please, ties into ^

Let me know there's a more acceptable place to store the pretty flag.

I kinda answered above, was that understandable?

Again, sorry about the late response

[emmabukacek]

@SimenB Hey, no problem! Thanks for the response! I'm sorry I'm also late, :grimacing: I'll get on this as soon as I can. So I need to:

• Rebase my branch onto V2
• Move the reporter into it's own place and have it set the actual reporter option instead of overloading the default behavior.
• Profit?

:smile:

[SimenB]

Profit after those 2 things, yeah!

[emmabukacek]

Hey @SimenB , so in the heart of async communication, I figured I'd let you know where I'm at and what I'm thinking.

So far, I've rebased my branch onto v2 (rendering most of my code bunk :laughing: ) and started undergoing the process to add the reporter into a separate file.

In the spirit of solving these issues:

• https://github.com/SimenB/stylint/issues/362
• https://github.com/SimenB/stylint/issues/354
• https://github.com/sertae/stylint-json-reporter/issues/3

This is the approach I'm taking in this PR:

• I'm creating a folder under src/ called formatters/.
• Inside, I'm creating the default formatter, default.js
  • Should this be like ESLint Compact (https://github.com/SimenB/stylint/issues/362, https://github.com/eslint/eslint/blob/master/lib/formatters/compact.js) or should this match what Stylint had before the updated formatter was introduced?
• Updating configuration to take formatter as an option instead of reporter.
  • Wrapping the formatter require in a try / catch block that throws a reasonable message in case the user provides a bad formatter.
• Tests

In the next PR (to make reviews manageable, but can be put into one), I would:

• Move and rename reporter.js to formatters/???.js
• Update tests

This is making some bigger architectural decisions, so feel free to shut me down, but I figured this was a little more comprehensive than what I originally introduced. Adding the formatters aligns with ESLint (which seems to be a common theme), helps add organization, and makes future formatters, i.e. json, a bit easier to bolt on.

Thoughts?

[SimenB]

Not so async, woo!

This is the approach I'm taking in this PR:

All of those points SGTM.

In the next PR (to make reviews manageable, but can be put into one), I would:

2 PRs also SGTM 😄

Thoughts?

You're awesome, thanks for stepping up!

Sorry about v2 killing a lot of your code, that's what I was sorta afraid of...

[SimenB]

@emmabukacek ProTip™️ run eslint . --fix from the root of the project, and it should cover up at least some of your CoffeeScript habits 🥇

I might add it as a precommit hook, but that might be a bit too much...

[emmabukacek]

Hey @SimenB , I'm just about done with this, but I'm at a bit of an impasse. Based on the tickets I've read, it appears that we want four ways to load in formatters:

• Passing in a native formatter, i.e. default
• Passing in the name of a module included as a project dependency, i.e. stylint-json-reporter
• Passing in the path to a file that serves as a custom formatter, i.e. my/really/cool/formatter.js
• Passing in a configuration object
  • Specifically thinking about this comment: https://github.com/SimenB/stylint/issues/354#issuecomment-252423748

Now this is fine and dandy if it comes from the .stylintrc file and with a few tweaks, all of these should work. However, how do we want to handle CLI arguments? Passing in a configuration isn't really realistic (JSON in the CLI sounds gross), but I think we could reasonably handle the first three if we're smart about it. I was thinking about having this syntax:

• Passing in a native formatter: stylint path/to/file --formatter=default
• Passing in a 3rd party formatter: stylint path/to/file --formatter=stylint-json-reporter
  • I need to update the code for this yet, but I'm thinking we look for a native formatter first, then through deps.
  • Could also use a different flag, like --third-party-formatter=, but that seems verbose.
• Passing in the path to a file for a formatter: stylint path/to/file --formatter=path/to/formatter.js
  • Could attempt native, 3rd party, then a specific path. ESLint looks for the presence of a / (my current code does that as well), but that means that you would have to write a root file as ./formatter.js instead of just formatter.js.
  • Same with a different flag, but not my favorite choice, i.e. --local-formatter=formatter.js

Anyway, just some thoughts. Let me know if you agree or have any ideas; I'm close to the finish line on this (I think, anyway :laughing: ) and I want to make sure I do it right.

Thanks!

[wojciechczerniak]

I would use --formatter, no need for specific commands.

Could attempt native, 3rd party, then a specific path.

👍

but that means that you would have to write a root file as ./formatter.js instead of just formatter.js

And that's ok. Some npm modules have dot in their name so formatter.js can still be a third party formatter module. Presence of / is obvious we deal with a path. There is an exception though , scoped modules @scope/project-name.

[emmabukacek]

@wojciechczerniak Yeah, that's a good point with the scoping. I'll try to throw together a solid series of tests to make sure we have our bases covered. Thanks for confirming for me!

[emmabukacek]

Okay, I think this is ready for an actual review! @SimenB @wojciechczerniak Happy to make any changes.

Once we're done here, I've got a few more PRs:

https://github.com/SimenB/stylint/pull/424 - formatterOptions -> formatter https://github.com/SimenB/stylint/pull/425 - A little clean-up

Obviously the latter is biased, but it looked like the repo was going towards spaces and I wanted to knock out that TODO. No hard feelings if it's not needed. 😄

[SimenB]

I'll review this later today, I swear!

[emmabukacek]

@SimenB Ugh, sorry to make you review the code twice, heh. I meant for this to be split from https://github.com/SimenB/stylint/pull/424 so it was easier to review, but somewhere along the lines, they were combined. :woman_facepalming: Closing this out.