Skip to content

Conversation

kitbellew
Copy link
Collaborator

Fixes #3786, as a final follow-on.

Copy link
Member

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

I have tested this in Scalafix and I managed to get all tests relying on the historical tokeniser behavior green 👍

@@ -19,7 +19,14 @@ object WhitespaceTokenizer {
def apply(input: Input, dialect: Dialect)(implicit
options: Option[TokenizerOptions],
tokens: java.util.Collection[Token]
): WhitespaceTokenizer = new Grouping(input, dialect)
): WhitespaceTokenizer =
if (options.forall(_.groupWhitespace)) new Grouping(input, dialect) else new Granular
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind to force custom options in Scalafix, but shouldn't Granular be the default to preserve the <4.9.8 behavior, at least in the 4.9.x line?

Suggested change
if (options.forall(_.groupWhitespace)) new Grouping(input, dialect) else new Granular
if (exists(_.groupWhitespace)) new Grouping(input, dialect) else new Granular

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -4,6 +4,7 @@ import scala.util.DynamicVariable

final class TokenizerOptions(
// options which control how tokens are emitted
val groupWhitespace: Boolean = true
Copy link
Member

Choose a reason for hiding this comment

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

same question as above, could it be an opt-in rather than an opt-out?

Suggested change
val groupWhitespace: Boolean = true
val groupWhitespace: Boolean = false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Also, let's once again make it default, as before scalameta#3786.
Copy link
Member

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

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

Tested on scalafix, tests are green out of the box, without any explicit options

@kitbellew kitbellew merged commit fb89693 into scalameta:main Aug 24, 2024
24 checks passed
@kitbellew kitbellew deleted the 3786_14 branch August 24, 2024 18:46
@xuwei-k
Copy link
Contributor

xuwei-k commented Oct 2, 2024

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.

3 participants