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

Update: add source property to LintResult object (fixes #7098) #7304

Merged
merged 2 commits into from Oct 13, 2016

Conversation

Projects
None yet
7 participants
@vitorbal
Copy link
Member

vitorbal commented Oct 4, 2016

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

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

This PR refers to #7098.

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)
Add a source property containing the entire file contents as a string into the linting results for each file that has linting errors. This can be used by formatters to output source code along with lint messages. This is a non-breaking change.

Is there anything you'd like reviewers to focus on?
A couple of important API decisions were done during the development of this PR in an effort to minimize redundancy and memory usage, and I would like to know if the rest of the team agrees with them:

  • The source property is omitted if no errors or warnings are present. As there is nothing to report, there is no need for the source code to be present.
  • The source property is also omitted if there were fixes and a output property is present. In this case, the source property would be redundant as the output property already contains the source code (also, the messages' column and line properties are updated to correspond to the location on the source code after the fixes)
@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 4, 2016

LGTM

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Oct 4, 2016

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

@jquerybot jquerybot added the CLA: Valid label Oct 4, 2016

@@ -240,7 +240,15 @@ The return value is an object containing the results of the linting operation. H
}
```

The top-level report object has a `results` array containing all linting results for files that had warnings or errors (any files that did not produce a warning or error are omitted). Each file result includes the `filePath`, a `messages` array, `errorCount`, `warningCount`, and optionally `output`. The `messages` array contains the result of calling `linter.verify()` on the given file. The `errorCount` and `warningCount` give the exact number of errors and warnings respectively on the given file. The `output` property gives the source code for the file with as many fixes applied as possible, so you can use that to rewrite the files if necessary. The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files.
The top-level report object has a `results` array containing all linting results for files that had warnings or errors (any files that did not produce a warning or error are omitted). Each file result includes:

This comment has been minimized.

Copy link
@vitorbal

vitorbal Oct 4, 2016

Author Member

Did some restructuring here while adding the documentation for the new source property, hope it's okay.

* `filePath` - Path to the given file.
* `messages` - Array containing the result of calling `linter.verify()` on the given file.
* `errorCount` and `warningCount` - The exact number of errors and warnings respectively on the given file.
* `source` - The source code for the given file. This property is omitted if this file has no errors/warnings or if the `output` property is present.

This comment has been minimized.

Copy link
@nzakas

nzakas Oct 4, 2016

Member

Hmmm, I'm not sure if omitting this when there's output is expected. Can you explain your thinking a bit more? I'm just worried about formatters all needing to check for both.

This comment has been minimized.

Copy link
@vitorbal

vitorbal Oct 5, 2016

Author Member

Thanks for the question!
My reasoning was that when fixes are applied, the source property would be redundant, since the output property already contains the source code.
Essentially, I thought we could omit the source property in that case in an effort to reduce memory usage. I do agree it complicates the API though, so I would be fine with not omitting in that case as well if we prefer.

This comment has been minimized.

Copy link
@nzakas

nzakas Oct 6, 2016

Member

Wouldn't source be the text before fixes are applied?

This comment has been minimized.

Copy link
@vitorbal

vitorbal Oct 7, 2016

Author Member

Sorry, I should have explained this as well from the get-go. Apologies for not being clear enough.

In my initial implementation, source was indeed the text before fixes.
But all of the error/warning messages in the messages property contain the line/column location after the fixes, so if we would use the source property in that case, the formatter would potentially report an incorrect location for each error.

As an example, let's say there are two messages, the first can be fixed, the second one cannot:

[{
    message: 'Expected line before comment',
    line: 4,
    column: 1
},
{
    message: '\'foo\' is defined but never used.',
    line: 5,
    column: 7
}]

After the first message is fixed, a newline is added above line 4 in the source code. The messages array is then updated to contain only one message:

[{
    message: '\'foo\' is defined but never used.',
    line: 6, // <--- line is updated to reflect fixes
    column: 7
}]

This means that the formatter would have to check for the presence of output in that case anyway, even if the source property would not be omitted when there are fixes.

This comment has been minimized.

Copy link
@nzakas

nzakas Oct 7, 2016

Member

Ah, I see, thanks for explaining. Okay, you've convinced me that it's okay to leave off source because of this. Can you please update the custom formatter documentation to make note of this?

This comment has been minimized.

Copy link
@SimenB

SimenB Oct 14, 2016

Contributor

Why does it specify that it's "omitted if this file has no errors/warnings", when the whole node is gone if no err/warns as noted in the paragraph above ("any files that did not produce a warning or error are omitted")?

@nzakas
Copy link
Member

nzakas left a comment

Can you please update the custom formatter documentation?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 7, 2016

@vitorbal I think this is probably ready after the docs changes, but I noticed you add the "do not merge" label. If you feel like you're ready, please remove that label.

@vitorbal

This comment has been minimized.

Copy link
Member Author

vitorbal commented Oct 7, 2016

@nzakas just pushed a new version that updates the custom formatters documentation!

I only added the "do not merge" label since the original issue (#7098) is still not marked as accepted. Do you think we could mark it as accepted now? Seems like no one manifested anything against the proposal after you posted the design summary.

@vitorbal vitorbal force-pushed the issue7098 branch from c7a9ecd to eff746e Oct 7, 2016

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 7, 2016

LGTM

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Oct 8, 2016

I marked original issue as accepted as there's enough support on that thread from the members of the team.

@vitorbal

This comment has been minimized.

Copy link
Member Author

vitorbal commented Oct 9, 2016

@ilyavolodin awesome, thanks!

@ilyavolodin ilyavolodin added the accepted label Oct 9, 2016

@vitorbal vitorbal force-pushed the issue7098 branch from eff746e to b384f80 Oct 9, 2016

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 9, 2016

LGTM

@vitorbal

This comment has been minimized.

Copy link
Member Author

vitorbal commented Oct 9, 2016

Pushed another version that fine-tunes the custom formatters doc changes a bit.

- **errorCount**: The number of errors for the given file.
- **warningCount**: The number of warnings for the given file.
- **source**: The source code for the given file. This property is omitted if this file has no errors/warnings or if the `output` property is present.
- **output**: The source code for the given file with as many fixes applied as possible. This property is omitted if no fix is available.

This comment has been minimized.

Copy link
@nzakas

nzakas Oct 10, 2016

Member

I'm not sure where, but we should probably mention that the source property on the individual messages will be going away and that people should instead use source or output from the top-level of the structure.

@nzakas

nzakas approved these changes Oct 10, 2016

Copy link
Member

nzakas left a comment

LGTM. Just one small note on the docs and I think this is ready to go!

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Oct 13, 2016

LGTM

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 13, 2016

@vitorbal looks good!

@btmills @mysticatea I think we should call this out in the release notes.

@nzakas nzakas merged commit b08fb91 into master Oct 13, 2016

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jQuery Foundation CLA All authors have signed the CLA
Details
@vitorbal

This comment has been minimized.

Copy link
Member Author

vitorbal commented Oct 13, 2016

@nzakas thanks for all the feedback on this one!

@vitorbal vitorbal deleted the issue7098 branch Oct 14, 2016

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.