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

Fix Scalastyle #1093

Merged
merged 1 commit into from
Jun 6, 2016
Merged

Fix Scalastyle #1093

merged 1 commit into from
Jun 6, 2016

Conversation

peterneyens
Copy link
Collaborator

Like @fthomas mentioned in #1092, we needed to update our scalastyle configuration because of scalastyle/scalastyle-sbt-plugin#47.

I used the same workaround as circe (circe/circe#156) and refined (fthomas/refined#122).

I am not sure if commonSettings is the right place in build.sbt to add it ?

@peterneyens peterneyens force-pushed the scalastyle-fix branch 2 times, most recently from 4b8d0df to 77f3de3 Compare June 5, 2016 21:31
@codecov-io
Copy link

codecov-io commented Jun 5, 2016

Current coverage is 88.89%

Merging #1093 into master will not change coverage

@@             master      #1093   diff @@
==========================================
  Files           226        226          
  Lines          2916       2916          
  Methods        2865       2865          
  Messages          0          0          
  Branches         48         48          
==========================================
  Hits           2592       2592          
  Misses          324        324          
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 00839fb...77f3de3

@@ -85,4 +86,5 @@ object StaticMethods {
}
true
}
// scalastyle:on magic.number
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be reading or understanding this incorrectly, but it looks like the scalastyle:off has return, but scalastyle:on has magic.number. Is this a mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a mistake indeed, thanks for spotting it !

@ceedubs
Copy link
Contributor

ceedubs commented Jun 6, 2016

Thanks! I left one minor question, but 👍 - scalastyle helps keep us consistent.

var ois: ObjectInputStream = null
// scalastyle:off on
Copy link
Member

Choose a reason for hiding this comment

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

This should be scalastyle:on null, right?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, it seems that you can also disable scalastyle on single lines with

var ois: ObjectInputStream = null // scalastyle:ignore null

see http://www.scalastyle.org/configuration.html.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for finding another of my errors. I didn't know you could also specify the checker in the single line comment.

@non
Copy link
Contributor

non commented Jun 6, 2016

It seems like a few of these rules (e.g. number.of.types) aren't really doing much for us. Does anyone think we should leave these rules on? I would be just as happy to turn them off as to disable them.

EDIT: I don't think @peterneyens has to necessarily "solve" this for us but since there's an open PR I figured I'd mention it here.

@peterneyens
Copy link
Collaborator Author

@non You are right, I figured it was best to get scalastyle working first and then worry about which rules we might have to change with some input from you guys.

The NumberOfTypes rule seems indeed a prime candidate to be removed from our scalastyle config. I would be happy to remove this and other rules in this PR if that's what we want.

@fthomas
Copy link
Member

fthomas commented Jun 6, 2016

👍 (when the build is green). Thanks @peterneyens!

I'd prefer merging this sooner than later and changing the scalastyle configuration in subsequent PRs. Is that okay for you, @non?

@ceedubs
Copy link
Contributor

ceedubs commented Jun 6, 2016

I agree that number of types is a prime candidate for rule removal in a separate PR, but I'm going to go ahead and merge this to get scalastyle rolling again. thanks again @peterneyens!

@ceedubs ceedubs merged commit e1a8257 into typelevel:master Jun 6, 2016
@peterneyens peterneyens deleted the scalastyle-fix branch June 8, 2016 23:09
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.

5 participants