Add support for linters #291

olafurpg · 2017-08-16T15:21:51Z

Just opening this PR to show progress

clean up commit history
move scalafix.internal.rewrite to separate PR (see Clean up repo in preparation for #291 #294 )
improve RewriteName, use List[Name] and Name has fields like .deprecated.
more linter tests
LintCategory.Info
remove akka from build import?
scalafix.lint docstrings

ShaneDelmore · 2017-08-17T07:21:05Z

This looks great so far 👍 New lint rules look very straightforward to implement.

olafurpg · 2017-08-17T07:23:44Z

Do you think it's confusing that a linter extends Rewrite? I've considered if we need a better name.

ShaneDelmore · 2017-08-17T07:40:28Z

That’s tricky as sometimes we will want a lint or be a rewrite (or have a rewrite associated) but not always. I’m heading off to bed but if I think of a better name I will let you know. In some ways this reminds me of a more powerful version of a classic regex. You have your capture groups, and optionally your replacement but that still doesn’t immediately suggest a better name to me.

It’s possible it could be generalized to some sort of Matcher with an optional Lint and an optional Rewrite to convey the idea that we always have to match a pattern of code, then we can either warn, rewrite, or both.

olafurpg · 2017-08-17T07:42:42Z

Borrowing terminology from pylint, we could use Checker (https://pylint.readthedocs.io/en/latest/how_tos/custom_checkers.html). I personally prefer sticking to Rewrite however.

gabro · 2017-08-17T08:09:39Z

I agree that Rewrite is confusing for rules that check only.

The confusion arises by the strange path we're taking: usually projects start as linters and then add some fixing capabilities, but scalafix started from the fixing part and we're retrofitting the linting.

Instead of having a unique concept, I would rather see two different entities: a Checker, Rule, or whatever and a Rewrite associated to it, but I'm not sure we have a clear way of drawing the line between the two.

WDYT?

olafurpg · 2017-08-17T08:22:01Z

I am not sure about separating rewrites from linters since that will leave us without a good name for rewrites that emit linter messages (like RemoveXmlLiterals in this PR). The API is identical for both linters and rewrites, so I would edge towards using a unified name. Maybe scalafix.Rule?

gabro · 2017-08-17T08:26:34Z

Rule sounds good. It may be useful (for future integrations or even for the readme) to include a flag that specifies whether this is a pure linting rule or whether it includes fixes as well.

olafurpg · 2017-08-17T08:36:44Z

Agreed, I'm leaning towards adding more metadata methods on Rewrite (or Rule if we go that route) such as

Rewrite.description: String
Rewrite.lintMessages: List[LintMessage]
...

gabro · 2017-08-17T08:38:12Z

OT: Preparing a talk on Scalafix is very entertaining these days. I need a Slidefix tool :P

olafurpg · 2017-08-17T08:38:28Z

I opened #293 to continue our discussion on renaming Rewrite

olafurpg · 2017-08-17T08:40:43Z

OT: Preparing a talk on Scalafix is very entertaining these days. I need a Slidefix tool :P

🤣

I apologize for the endless changes, I'm not good at sitting down and designing thoroughly before writing code.

gabro · 2017-08-17T08:42:57Z

I apologize for the endless changes, I'm not good at sitting down and designing thoroughly before writing code.

No apologies needed. I would be more worried about an exciting project slowing down in its early stages because of minor compatibility issues (just don't rename stuff 10 minutes before my talk! I'll keep an eye on you 👀 )

Clean up repo in preparation for #291

olafurpg · 2017-08-17T14:57:16Z

All right, I think this PR is ready now :)

I ended up doing a lot of testing infrastructure changes in this PR. This may pollute the actual diff but it gives me a lot more confidence the changes work as intended. I am super happy with the CliTest rewrite and the lint message assertions via comments in scalafix-testkit are quite nice too.

The main data structures went through some renaming in the final commits while I was writing docs

LintCategory: a certain kind/type of lint message
LintMessage: a (message, position, LintCategory) triplet.

Escape hatches #241 are still a TODO, but those will have to wait for another PR.

RewriteName is a wrapper around a non-empty list of strings instead of being a single string. Original names are preserved when composing multiple RewriteNames.

New package scalafix.lint with data structures for emitting messages from linters: - LintID: uniquely identifies a certain kind of linter message, owned by a rewrite. A LintID is attached to a default warning/error severity but can be configured to another category with `lint.ignore/warning/error = [ LintID ]` - LintMessage: an instance of LintID with a customized message at a particular position. A LintMessage can be created wit LintID.at(String/Position). - A LintMessage can be turned into a Patch with ctx.lint(LintMessage). LintMessages don't report to the console until Rewrite.apply is called. To accommodate linter development, scalafix-testkit now enforces that expected warnings/errors are documented in the input sources with a trailing `// scalafix: error/warning` comment. scalafix-cli returns a non-zero exit code of kind "LinterError" when a linter reports an error.

- RewriteName is a list of RewriteIdentifier. - RewriteIdentifier is a name + optional deprecation warning. This makes it possible to rename a rewrite without breaking peoples config. - Rewrite.empty has name RewriteName.empty, which is an empty list of RewriteIdentifier.

This change allows rewrites to change their names without breaking compatibility. Users using the old name will receive deprecation warnings and have opportunity to switch to the new name.

unit/test has fixtures to build SemanticCtx, which will come in handy to test the cli.

The cli tests have in need for a rewrite for a long time. This commits creates a helper method to easily construct a fixture and run assertions against the expected output.

Instead of using LintID.owner, the RewriteCtx.printMessages takes an `owner: RewriteName` argument. This results in a simpler LintID data structure.

"Category" is misleading.

LintCategory is more suitable since it is a category of LintMessages.

I've been meaning to rename this rewrite for a while but wasn't satisfied that this would break people's configuraiton. I wanted to solve this potentially common problem in the future more elegantly. Now that rewrites can have multiple names and names can have deprecation warnings, this commits tests that it indeed works as expected.

- doc polish - Require `// scalafix: Rewrite.LintCategory` assertions instead of warning/error. This is more robust. - Rename VolatileLazyVal to DottyVolatileLazyVal in tests, I noticed because of deprecation warnings getting printed out. - Don't print out lint messages in scalafix-testkit by default, if you have a lot of assertions then the console gets flooded with too many messages.

- remove unused imports with scalafix (hurray!) - refactor build.sbt a bit - polish new doc entries - Remove unused Patch.applyInternal

Reported at scalameta/scalameta#1072

olafurpg · 2017-08-18T08:50:48Z

I will go ahead and merge this to unblock other work that relies on the new cli testing infrastructure. I am happy to continue iterating on the lint API :)

olafurpg mentioned this pull request Aug 16, 2017

Support linters, rewrites that only emit info/warn/error messages #92

Closed

olafurpg mentioned this pull request Aug 17, 2017

Rename Rewrite to Checker/Rule? #293

Closed

olafurpg mentioned this pull request Aug 17, 2017

Tuple enrichments are deprecated in 0.13 but not in 1.0 sbt/sbt#3454

Closed

olafurpg added a commit that referenced this pull request Aug 17, 2017

Merge pull request #294 from olafurpg/internal.rewrite

0de37da

Clean up repo in preparation for #291

olafurpg force-pushed the messages branch from c3605c3 to 664fcba Compare August 17, 2017 11:18

olafurpg changed the title ~~[WIP] Add infrastructure for Linters~~ Add support for linters Aug 17, 2017

olafurpg force-pushed the messages branch from a6f474d to 9e18eaf Compare August 17, 2017 13:45

olafurpg mentioned this pull request Aug 17, 2017

Add tests for --out-from and --out-to #296

Closed

olafurpg added 9 commits August 18, 2017 10:23

Replace sourcecode.Name with scalafix.rewrite.RewriteName.

a9ff6a0

RewriteName is a wrapper around a non-empty list of strings instead of being a single string. Original names are preserved when composing multiple RewriteNames.

Fix failing tests and compilation errors.

a014ff4

Fix bug where cli always returned LinterError.

6e5b915

Add LintCategory.Info

9f11786

Add docstrings for new scalafix.lint data structures.

33ec950

Emit deprecation warnings when referencing a rewrite by old name.

9aec7a0

This change allows rewrites to change their names without breaking compatibility. Users using the old name will receive deprecation warnings and have opportunity to switch to the new name.

Move cli/test to unit/testOnly *cli*.

dcacac5

unit/test has fixtures to build SemanticCtx, which will come in handy to test the cli.

olafurpg added 8 commits August 18, 2017 10:23

Rewrite CliTest with StringFS.

6433616

The cli tests have in need for a rewrite for a long time. This commits creates a helper method to easily construct a fixture and run assertions against the expected output.

Remove LintID.owner.

e43b4b0

Instead of using LintID.owner, the RewriteCtx.printMessages takes an `owner: RewriteName` argument. This results in a simpler LintID data structure.

Document lint messages on the website

c0bd9f4

s/LintCategory/LintSeverity/

275bdf0

"Category" is misleading.

s/LintID/LintCategory/

5d38cf6

LintCategory is more suitable since it is a category of LintMessages.

Clean up before merge

9798839

- remove unused imports with scalafix (hurray!) - refactor build.sbt a bit - polish new doc entries - Remove unused Patch.applyInternal

olafurpg force-pushed the messages branch from 9e27ebe to 9798839 Compare August 18, 2017 08:23

Disable sbt-scalafix due to scaladoc problems

b8e85db

Reported at scalameta/scalameta#1072

olafurpg merged commit b42cf5c into scalacenter:master Aug 18, 2017

olafurpg deleted the messages branch August 18, 2017 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for linters #291

Add support for linters #291

olafurpg commented Aug 16, 2017 •

edited

Loading

ShaneDelmore commented Aug 17, 2017

olafurpg commented Aug 17, 2017 •

edited

Loading

ShaneDelmore commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

olafurpg commented Aug 18, 2017

Add support for linters #291

Add support for linters #291

Conversation

olafurpg commented Aug 16, 2017 • edited Loading

ShaneDelmore commented Aug 17, 2017

olafurpg commented Aug 17, 2017 • edited Loading

ShaneDelmore commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

olafurpg commented Aug 17, 2017

gabro commented Aug 17, 2017

olafurpg commented Aug 17, 2017

olafurpg commented Aug 18, 2017

olafurpg commented Aug 16, 2017 •

edited

Loading

olafurpg commented Aug 17, 2017 •

edited

Loading