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

Add more scalastyle rules #1142

Merged
merged 8 commits into from
Jun 24, 2016
Merged

Add more scalastyle rules #1142

merged 8 commits into from
Jun 24, 2016

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Jun 18, 2016

The intention of this is so that potential contributors see a consistent
style and so that they can check the scalastyle output and hopefully
avoid reviewers leaving nitpicky formatting comments on PRs.

I'd like to wait until #845 is merged before we merge this one so we don't potentially cause yet another merge conflict in that PR. I wanted to go ahead and submit this so that I don't forget to though.

The intention of this is so that potential contributors see a consistent
style and so that they can check the scalastyle input and hopefully
avoid reviewers leaving nitpicky formatting comments on PRs.
@codecov-io
Copy link

codecov-io commented Jun 18, 2016

Current coverage is 88.80%

Sunburst

No coverage report found for master at c869d7b.

Powered by Codecov. Last updated by c869d7b...62b662f

@peterneyens
Copy link
Collaborator

Maybe we could also add a rule to check we write type class instead of typeclass, so we don't need to clean this up regularly like in #1149.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 19, 2016

@peterneyens that is an excellent idea. I'm very inconsistent on this point.

I think that there are a couple things we'll have to keep in mind.

  • I don't think scalastyle runs on our docs, so it's still up to us to try to be consistent there.
  • We'll probably have to make the pattern not match if there's a leading @ to avoid our simulacrum annotations tripping this.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 19, 2016

@peterneyens updated to include "type class" rule.

@peterneyens
Copy link
Collaborator

peterneyens commented Jun 20, 2016

Maybe we could also remove the NumberOfTypes rule in this PR, like we mentioned in #1093 ? Or should we create a new PR to remove rules ?

@ceedubs ceedubs changed the title [DO NOT MERGE] Add more scalastyle rules Add more scalastyle rules Jun 23, 2016
@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 23, 2016

#845 has been merged and I've updated this PR. I think it's ready for review/merge.

@kailuowang
Copy link
Contributor

👍 LGTM

@@ -27,7 +27,7 @@ final case class OneAnd[F[_], A](head: A, tail: F[A]) {
*/
def filter(f: A => Boolean)(implicit F: MonadCombine[F]): F[A] = {
val rest = F.filter(tail)(f)
if (f(head)) F.combineK(F.pure(head), rest) else rest
if(f(head)) F.combineK(F.pure(head), rest) else rest
Copy link
Member

Choose a reason for hiding this comment

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

Later in this diff, spaces are inserted between if and (, for example here. Can we add a scalastyle rule to make this consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow good catch. The other places were caught by EnsureSingleSpaceAfterTokenChecker. It seems to me like a scalastyle bug that this particular place isn't caught by that rule.

@fthomas
Copy link
Member

fthomas commented Jun 24, 2016

I made a minor comment about spaces after if. Apart from that, this LGTM. 👍

This should be caught by `EnsureSingleSpaceAfterTokenChecker` but for
some reason it's not in some places.
@ceedubs
Copy link
Contributor Author

ceedubs commented Jun 24, 2016

@fthomas I've added a rule for space after if since for some reason EnsureSingleSpaceAfterTokenChecker wasn't working in certain cases.

@fthomas fthomas merged commit d256ec3 into typelevel:master Jun 24, 2016
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