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 type annotations for public members with Scalafix #1034

Merged
merged 4 commits into from Nov 2, 2019

Conversation

@olafurpg
Copy link
Member

olafurpg commented Oct 31, 2019

Previously, Scalafix only checked for unused imports. Now, it also adds
type annotations for public members.

olafurpg added 3 commits Oct 31, 2019
Previously, Scalafix only checked for unused imports. Now, it also adds
type annotations for public members.
To avoid redundant type annotations.
I previously ran `scalafix`, which only runs on the Compile
configuration. The `scalafixAll` command runs also in tests.
@olafurpg olafurpg requested review from tgodzik and gabro Oct 31, 2019
Copy link
Member

gabro left a comment

Overall, I like the change and Scalafix is doing an impressive job :O

My main concerns are:

  • in some cases the annotation is borderline noisy (I've highlighted a few)
  • this will cause developers to forget the annotation in many cases and then seeing the CI turn red

Ideally, adding an annotation should be an editor shortcut (Scalafix in the editor, anyone? :D), but since we're not there yet, can we maybe:

  • turn off annotations in some trivial cases
  • add a pre-commit step (not sure if it's too heavy)
@@ -19,7 +19,8 @@ import scala.meta.internal.mtags.GlobalSymbolIndex
* Handles both javadoc and scaladoc.
*/
class Docstrings(index: GlobalSymbolIndex) {
val cache = TrieMap.empty[String, SymbolDocumentation]
val cache: TrieMap[String, SymbolDocumentation] =

This comment has been minimized.

Copy link
@gabro

gabro Nov 1, 2019

Member

I’m not sure I see the value in cases like this, is there an option to avoid the annotation in trivial cases?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 1, 2019

Author Member

I agree this particular case is quite noisy, refactored this code to use new TrieMap[K, V](), which the rewrite allows to be unannotated.

val OPEN_TAG = "^<([A-Za-z]+)( [^>]*)?(/?)>$".r
val CLOSE_TAG = "^</([A-Za-z]+)>$".r
val OPEN_TAG: Regex = "^<([A-Za-z]+)( [^>]*)?(/?)>$".r
val CLOSE_TAG: Regex = "^</([A-Za-z]+)>$".r

This comment has been minimized.

Copy link
@gabro

gabro Nov 1, 2019

Member

these are other examples where I don't see a great value.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 1, 2019

Author Member

Sure, but this particular is not a major regression either IMO. Once you get used to always having public types I personally find it quite helpful even in cases like this, now you don't even need to squint to observe the trailing .r

Copy link
Member Author

olafurpg left a comment

Thank you for the review! I agree the rewrite is occasionally a bit noisy but I think its overall an improvement to code readability.

A git pre-push hook would be too slow, since this requires compilation. It works just like the "remove unused imports" rewrite we have had enabled for a while now. We might want to start using sbt-scalafmt so that we can write an sbt command that runs scalafix AND scalafmt in one go.

val OPEN_TAG = "^<([A-Za-z]+)( [^>]*)?(/?)>$".r
val CLOSE_TAG = "^</([A-Za-z]+)>$".r
val OPEN_TAG: Regex = "^<([A-Za-z]+)( [^>]*)?(/?)>$".r
val CLOSE_TAG: Regex = "^</([A-Za-z]+)>$".r

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 1, 2019

Author Member

Sure, but this particular is not a major regression either IMO. Once you get used to always having public types I personally find it quite helpful even in cases like this, now you don't even need to squint to observe the trailing .r

@@ -19,7 +19,8 @@ import scala.meta.internal.mtags.GlobalSymbolIndex
* Handles both javadoc and scaladoc.
*/
class Docstrings(index: GlobalSymbolIndex) {
val cache = TrieMap.empty[String, SymbolDocumentation]
val cache: TrieMap[String, SymbolDocumentation] =

This comment has been minimized.

Copy link
@olafurpg

olafurpg Nov 1, 2019

Author Member

I agree this particular case is quite noisy, refactored this code to use new TrieMap[K, V](), which the rewrite allows to be unannotated.

@gabro
gabro approved these changes Nov 2, 2019
Copy link
Member

gabro left a comment

Thanks for the reply! I’m only concerned it will be annoying to forget annotations in trivial expressions only to see the CI turn red.

But let’s give this a spin and see for ourselves:)

@olafurpg olafurpg closed this Nov 2, 2019
@olafurpg olafurpg reopened this Nov 2, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 2, 2019

A agree, if it turns out to be more annoying than helpful then we can turn it off. My motivation for exploring this partly comes from wanting to see if we can somehow use the new -Youtline to improve

  • time to completions in new workspaces
  • keeping the symbol table up to date for the presentation compiler during refactorings when the code doesn't compile and it off-sync with the classfiles on disk
@olafurpg

This comment has been minimized.

Copy link
Member Author

olafurpg commented Nov 2, 2019

If we're going to ask users to type annotate their public members then we should first try it out ourselves

@gabro

This comment has been minimized.

Copy link
Member

gabro commented Nov 2, 2019

Ah, that’s very interesting. Makes sense 👍

@tgodzik
tgodzik approved these changes Nov 2, 2019
Copy link
Collaborator

tgodzik left a comment

Looks good, I think some more members might be made public, but I don't think we need to do it here.

Thanks Olaf!

@tgodzik tgodzik merged commit 7d33c4b into scalameta:master Nov 2, 2019
18 checks passed
18 checks passed
Windows unit tests
Details
Windows unit tests
Details
Linux unit tests
Details
Linux unit tests
Details
Sbt integration
Details
Sbt integration
Details
Maven integration
Details
Maven integration
Details
Gradle integration
Details
Gradle integration
Details
Mill integration
Details
Mill integration
Details
Slow tests
Details
Slow tests
Details
Scala cross tests
Details
Scala cross tests
Details
Scalafmt/Scalacheck/Docs
Details
Scalafmt/Scalacheck/Docs
Details
@olafurpg olafurpg deleted the olafurpg:explicit-result-types branch Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.