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

feat: Add support for external output formatters #77

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

relekang
Copy link
Owner

@relekang relekang commented Sep 3, 2016

Fixes #25

Copy link
Collaborator

@trygveaa trygveaa left a comment

Choose a reason for hiding this comment

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

Looks good to me.

-f, --format [format] The output format
-b, --branch [branch] The branch to diff against
-h, --help Output usage information
-V, --version Output the version number
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the indent change and the capital O's, this doesn't match the output of the command anymore.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm, should make a way to keep this the same as the output.

@@ -78,6 +80,34 @@ below.

* `list-files` - list the files in the current diff that lint-filter will use. Nice for faster linting.

### External formatters
An external formatter should export a function takes two arguments(input and stats) and returns
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a space after arguments.

### External formatters
An external formatter should export a function takes two arguments(input and stats) and returns
a string with the formatted output. Below are examples of the structure that the two arguments
will be on. Input is a list of files with lint messages and stats is a object with severity as key
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/a object/an object/

if (!{}.hasOwnProperty.call(formatters, format)) {
throw new Error(`Formatter with name '${format}' does not exist.`)
let formatter
if (/^require:/.test(format)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking it could try to load the external formatter if the name is not one of the builtin ones, instead of having to specify require, but maybe it's better to be explicit?

Copy link
Owner Author

@relekang relekang Sep 20, 2016

Choose a reason for hiding this comment

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

I did it explicitly because require is very slow and most use cases is probably going to use an internal one. Also being explicit is good thing ✌️

Copy link
Collaborator

Choose a reason for hiding this comment

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

You would only call require if the name is not one of the builtins, so it shouldn't matter that it is slow. But sure, being explicit is fine.

@relekang relekang force-pushed the feat/external-output-formatter branch from c011b53 to 85256f8 Compare December 12, 2016 09:55
@yannickcr
Copy link
Contributor

Hi! Any news on this ? I would love to get this feature 😃

@relekang
Copy link
Owner Author

Cool, I had kind of forgotten about this or not prioritised it. If you are interested, I am propably able to do it this week, as in today or tomorrow.

Btw, what format are you interested in?

@yannickcr
Copy link
Contributor

The main issue we have today with the text formatter is that the errors and warnings are indistinguishable, both are displayed the same way. This is problematic to know what really needs to be fixed.

This particular issue can be fixed by updating the text formatter, but having the possibility to make our own would give us much more freedom.

@relekang
Copy link
Owner Author

This sounds like a good thing to fix in the text reporter in addition to landing this feature. Are you up for submitting a PR for the text formatter?

@yannickcr
Copy link
Contributor

No problem, I'll try to submit a PR for this today.

@relekang relekang force-pushed the feat/external-output-formatter branch from 85256f8 to 73bb35a Compare January 18, 2017 17:27
@relekang relekang merged commit c5d0048 into master Jan 18, 2017
@relekang relekang deleted the feat/external-output-formatter branch January 18, 2017 17:33
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.

None yet

3 participants