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

New: Codeframe (JSCS) formatter (fixes #5860) #7437

Merged
merged 1 commit into from Oct 28, 2016

Conversation

vitorbal
Copy link
Member

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

[x] Add something to the core

What changes did you make? (Give an overview)
Implements the codeframe formatter, which closely resembles the default JSCS formatter. Here are some screenshots of how it looks like under different circumstances:

When only errors are reported:
screenshot 2016-10-21 23 15 31

When errors and warnings are reported:
screenshot 2016-10-21 23 15 54

When only warnings are reported:
screenshot 2016-10-21 23 16 51

When a parsing error is reported:
screenshot 2016-10-21 23 18 39

When an error that has no line/column is reported:
screenshot 2016-10-21 23 20 01

Is there anything you'd like reviewers to focus on?

  • Is this a "Core" or a "New" change? I was not sure :(
  • I took the liberty of suggesting a name for the formatter that is not "JSCS", as I believe it would be a confusing name after a year or two from now. More than happy to rename to something different or keep "JSCS", if we prefer!
  • I tried to format the filepath in a way that it shows more than just the filename (for more context), but relative to the current working directory (to avoid super long paths).
  • I also added the line:column format to the end of the filepath so editors can open the file directly on the line/column of the error.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@vitorbal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @platinumazure and @gyandeeps to be potential reviewers.

@vitorbal vitorbal added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels Oct 23, 2016
@vitorbal vitorbal added this to the JSCS Compatibility milestone Oct 23, 2016

if (sourceCode) {
result.push(
codeFrame(sourceCode, message.line, message.column, { highlightCode: false })
Copy link
Member Author

Choose a reason for hiding this comment

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

disabled code highlighting as the performance impact was way too high.

const summary = [];

if (errors > 0) {
summary.push(`${errors} code style ${pluralize("error", errors)}`);
Copy link
Member

Choose a reason for hiding this comment

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

This is so great! Looking forward to using this :) Don't have time to do a full review right now (will try to get back to this soon), but my preference would be for this to just be error and warning instead of code style error and code style warning, since not all reported errors/warnings are style related.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!! That's a good point, I can change it to say only 2 errors found., 1 error and 2 warnings found., etc instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kaicataldo removed the code style part of the sentence, as discussed!

@eslintbot
Copy link

LGTM

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.

This looks way cool!

Just two questions:

  1. What does the output look like if this is piped into an output file?
  2. Can this be adapted to support end locations (although perhaps only when the end location is on the same line)?

@kaicataldo
Copy link
Member

Any particular reason why it needs to have an end location? I don't think JSCS's formatter did that either.

@platinumazure
Copy link
Member

Nope! I just figured since we were already making enhancements, I thought
I'd ask about it.

On Oct 27, 2016 9:56 PM, "Kai Cataldo" notifications@github.com wrote:

Any particular reason why it needs to have an end location? I don't think
JSCS's formatter did that either.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7437 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARWeuW4zDy8cSL4S2dK7_AbnQ6Rrqyyks5q4WRZgaJpZM4KeNvz
.

@ilyavolodin ilyavolodin merged commit e46666b into eslint:master Oct 28, 2016
@hzoo
Copy link
Member

hzoo commented Oct 28, 2016

🎉 nice job @vitorbal!

@vitorbal
Copy link
Member Author

@hzoo Thanks! babel-code-frame worked great :)

@vitorbal
Copy link
Member Author

@platinumazure sorry I couldn't get to your questions before.

Here's how it looks like when the output is piped to a file:

error: Strings must use doublequote (quotes) at src/test.js:2:11:
  1 | reply({
> 2 |     type: 'text',
    |           ^
  3 |     text: `*${plugin.name}* - ${plugin.description}\n\n${plugin.help}`,
  4 |     options: {
  5 |         parse_mode: "Markdown"


error: Unexpected trailing comma (comma-dangle) at src/test.js:6:6:
  4 |     options: {
  5 |         parse_mode: "Markdown"
> 6 |     },
    |      ^
  7 | })
  8 | 


2 errors found.

The color escape codes are not there, which is nice!

Regarding end locations, that's a good idea, but babel-code-frame doesn't support such feature. It might be an interesting enhancement in the future though, since we're planning to update a bunch of rules to report a location range (I think @mysticatea is the one working on that, AFAIR).

@vitorbal vitorbal deleted the issue5860 branch October 29, 2016 09:30
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@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 Feb 6, 2018
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 core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants