Page MenuHomePhabricator

Add a Scalastyle linter
Needs RevisionPublic

Authored by josh-newman on Sep 3 2014, 8:12 AM.

Details

Summary

I've been using Phabricator a lot recently for Scala projects. We thought it might be useful to add linting to our code review process, and I didn't see any current Scala linters in Arcanist, so I put together a Scalastyle linter. I didn't find existing revisions or tasks related to Scala linter configuration, but if there are I'm happy to wait or contribute to that instead of pushing this change.

I'm not too familiar with Arcanist contribution processes; please let me know if I'm doing anything very wrongly!

Test Plan

I ran this linter locally.

I tried adding a unit test but I wasn't sure how to configure the paths to the Scalastyle JAR and configuration file in the test environment. Are there examples of other tests that do this?

Diff Detail

Repository
rARC Arcanist
Branch
scalastyle
Lint
Lint OK
Unit
Unit Test Errors
Build Status
Buildable 2586
Build 2590: [Placeholder Plan] Wait for 30 Seconds

Unit TestsFailed

Excuse: Not new failure.
TimeTest
0 mstestPHPLint
0 mstestEverythingImplemented
0 mstestFixLetterCase
0 mstestJSONLinter
0 mstestJscsLinter
View Full Test Results (1 Failed · 14 Passed · 14 Skipped)

Event Timeline

josh-newman updated this revision to Diff 25036.Sep 3 2014, 8:12 AM
josh-newman retitled this revision from to Add a Scalastyle linter.
josh-newman updated this object.
josh-newman edited the test plan for this revision. (Show Details)
josh-newman added a reviewer: epriestley.

@epriestley, is there anyone else I should send this revision to for review? (I just picked your name out of the contribution guide.) Thanks!

chad added a subscriber: chad.Sep 10 2014, 2:25 AM

We have about 80 revisions in the queue for review, so it may take some time. For a change to go in, we need to be able to fully test, document, and support it in the upstream, so it's not a quick process. Sorry. :(

josh-newman edited edge metadata.
  • Add check for empty stdout.
turadg added a subscriber: turadg.
rafikk requested changes to this revision.Sep 29 2014, 2:11 PM
rafikk added a reviewer: rafikk.
rafikk added a subscriber: rafikk.

This linter should offer an option to use the ScalaStyle SBT plugin.

This revision now requires changes to proceed.Sep 29 2014, 2:11 PM
chad removed a reviewer: rafikk.Sep 29 2014, 2:16 PM

Maybe Scala Abide is a better choice for scala linter because it is maintained by Scala Official Repository

Thanks for the reference, @hanfeisun. It looks like scala-abide isn't released yet, though; do you know when it'll be ready for general use?

Thanks for the reference, @hanfeisun. It looks like scala-abide isn't released yet, though; do you know when it'll be ready for general use?

https://github.com/scala/scala-abide/issues/43#issuecomment-76624724

He said there isn't an ETA yet, but not far from a 0.1 release. will have a better estimate after ScalaDays (in 2 weeks)..

joshuaspence requested changes to this revision.Jun 1 2015, 10:02 PM
joshuaspence edited edge metadata.

Some initial feedback.

src/lint/linter/ArcanistScalastyleLinter.php
13

This should be wrapped in a call to the pht() function so that it is translatable.

32–34

This is the default behaviour, so you don't need to override this method explicitly.

37–44

You should be able to remove the optional type specifier from getLinterConfigurationOptions() instead.

47

Prefer '-jar='.$this->jarPath

48

As above.

49

Prefer --quiet (without true).

52–54

Just remove this.

113

Should be namespaced as scalastyle.jar

114

Why a list of strings?

119

As above.

120

Why a list of strings?