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

Preparing for enabling -Wall #282

Merged
merged 3 commits into from May 13, 2019

Conversation

Projects
None yet
2 participants
@blast-hardcheese
Copy link
Collaborator

commented May 4, 2019

I'm interested in getting ahead of code quality issues by ensuring that all warnings are treated as errors, as well as enabling select wartremover warts.

This PR contains some initial work towards that, by at least removing unused imports, as well as minor formatting.

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:preparing-for-Wall branch from da3cd60 to 3b80bfb May 5, 2019

@tomasherman

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

How do you feel about cats.implicits._ ... it's my understanding that it might be better to use explicits imports as it makes compilation/ide support faster... i actually tried to do a PR with this by my cats knowledge left me hanging at some files. .. is that something you would like to be solved?

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2019

I'm on the fence about cats.implicits._. Initially I was very much against it, but over the years I've come to value the ease of having all instances available when I need them. Additionally, I find that it lowers the barrier to entry to folks so they don't need to remember cats.instances.traverse._ or cats.instances.foldable._ in order to do something.

I generally have the cats source open if I find that what I'm doing is too much boilerplate, evaluating whether someone else has already solved the same problem, but I guess some folks use IDEs to solve the same problem, by using completion or otherwise browsing available methods. As I prefer vim, I don't have a preference one way or another, as it's just another compiler error, easy enough to resolve and it happens fairly infrequently.

my cats knowledge left me hanging at some files

What do you mean by this, you couldn't identify which implicits were necessary?

@tomasherman

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

What do you mean by this, you couldn't identify which implicits were necessary?

Yes, i think it was this file: modules/codegen/src/main/scala/com/twilio/guardrail/WriteTree.scala

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:preparing-for-Wall branch from 3b80bfb to 4ef618d May 11, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

@tomasherman I took a look:

[error] .../guardrail/modules/codegen/src/main/scala/com/twilio/guardrail/WriteTree.scala:26:19: value traverse is not a member of Option[Int]
[error]           diffIdx.traverse { diffIdx =>
[error]                   ^

requires cats.instances.option._ to adapt the stdlib to cats, as well as cats.syntax.traverse._

[error] .../guardrail/modules/codegen/src/main/scala/com/twilio/guardrail/WriteTree.scala:28:28: could not find implicit value for evidence parameter of type cats.Applicative[[V]cats.data.Writer[List[String],V]]
[error]           diffIdx.traverse { diffIdx =>
[error]                            ^

requires cats.instances.list._, as there's only an instance for Applicative for Writer if L (List[String]) has an instance of Monoid[_]

[error] .../guardrail/modules/codegen/src/main/scala/com/twilio/guardrail/WriteTree.scala:47:27: value map is not a member of com.twilio.guardrail.WriteTree => cats.data.Writer[List[String],java.nio.file.Path]
[error]     unsafeWriteTreeLogged.map {
[error]                           ^

requires cats.instances.function._ and cats.syntax.functor._ as it's trying to map over a Function1[A, B]

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

I think this is an indicator that whatever performance gains we'd get are not worth the mental burden on potential contributors, unless compilation speed improved by orders of magnitude.

I'd love to see benchmarks, if you've got links 😀

@tomasherman

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I don't have any, it's just a feeling that all-you-can-eat import slowers down intellij and scalac :) Thanks for taking the time to take a look at this!

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator Author

commented May 13, 2019

Definitely onboard for revisiting this in the future, thanks for raising the concern!

@blast-hardcheese blast-hardcheese merged commit 3066dfd into twilio:master May 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blast-hardcheese blast-hardcheese deleted the blast-hardcheese:preparing-for-Wall branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.