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

Remove warnings #3807

Merged
merged 17 commits into from Dec 19, 2017

Conversation

Projects
None yet
2 participants
@dwijnand
Member

dwijnand commented Dec 12, 2017

No description provided.

dwijnand added some commits Dec 4, 2017

@dwijnand dwijnand requested a review from eed3si9n Dec 12, 2017

@dwijnand dwijnand added this to the 1.2.0 milestone Dec 12, 2017

@eed3si9n

A few comments.

doc: JavaTools,
log: Logger,
reporter: Reporter,
): Gen = ???

This comment has been minimized.

@eed3si9n

eed3si9n Dec 13, 2017

Member

Why ??? instead of removing it or delegating it to somewhere?

This comment has been minimized.

@dwijnand

dwijnand Dec 13, 2017

Member

It's public API, and I think a non-fatal runtime error, that can be handled, is better than a fatal linking error.

This comment has been minimized.

@eed3si9n

eed3si9n Dec 14, 2017

Member

I don't think we should break this until sbt 2.x unless there's a really good reason for it.

This comment has been minimized.

@dwijnand

dwijnand Dec 14, 2017

Member

It's already broken, I just made it more obviously so (and got rid of a bunch of unused warnings in the process):

https://github.com/sbt/sbt/blob/v1.1.0-RC1/main-actions/src/main/scala/sbt/Doc.scala#L63

- conflict: sbt.AI\$R is enabled by sbt.AI\$Q; excluded by sbt.AI\$T""")
deducePlugin(T && U, log) must throwAn[AutoPluginException](
message = s"""Contradiction in enabled plugins:
- requested: sbt.A\\$$T && sbt.A\\$$U

This comment has been minimized.

@eed3si9n

eed3si9n Dec 13, 2017

Member

Should this be AI\\$$T etc?

This comment has been minimized.

@dwijnand

dwijnand Dec 13, 2017

Member

Well spotted, I messed up my find/replace - fixed.

dwijnand added some commits Dec 4, 2017

@dwijnand

This comment has been minimized.

Member

dwijnand commented Dec 14, 2017

This is now green, so it's ready for review.

Note that Codacy is red because of 3 cases of lines where I didn't introduce the issue (catching Exception) and 1 case of not putting a call-by-name parameter in the last position, which I think is the right decision in this case.

@eed3si9n eed3si9n merged commit 34d311f into sbt:1.x Dec 19, 2017

1 of 2 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n removed the in progress label Dec 19, 2017

@dwijnand dwijnand deleted the dwijnand:remove-warnings branch Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment