Skip to content

Standardize -rewrite advertisements and suppress them in REPL #14955

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

Merged
merged 2 commits into from
May 11, 2022

Conversation

griggt
Copy link
Contributor

@griggt griggt commented Apr 18, 2022

Extracted from #14294

@michelou
Copy link
Contributor

@griggt Nice refactoring (String -> SourceVersion, 3 rewriteNotice -> 2 rewriteNotice, REPL vs. non interactive mode) !

@griggt
Copy link
Contributor Author

griggt commented May 9, 2022

ping for review

@griggt griggt force-pushed the standardize-rewrite-ads branch from 4696622 to 62b2138 Compare May 9, 2022 19:58
Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

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

I was just figuring out rewrite infrastructure for -Yrewrite-imports. I haven't quite reached the point of recommending they use it, but I will soon be there.

report.warning(
ex"""pattern's type ${pat.tpe} $problem the right hand side expression's type $reportedPt
|
|If the narrowing is intentional, this can be communicated by $fix.${err.rewriteNotice}""",
|If the narrowing is intentional, this can be communicated by $fix.$rewriteMsg""",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry my comment will be out of scope, but as a feature request, I wonder if there is a syntax to make string construction more intuitive or easier to read. Just thinking out loud, what if the rewrite message were given so that it needn't be stuck at the end by hand.

@bishabosha bishabosha added this to the 3.2.0-RC1 milestone May 9, 2022
7 | 2 foo 4 // error
| ^^^
| Alphanumeric method foo is not declared `infix`; it should not be used as infix operator.
| Instead, use method syntax .foo(...) or rewrite as `foo`.
Copy link
Member

@bishabosha bishabosha May 9, 2022

Choose a reason for hiding this comment

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

Im really not sure what "or rewrite as foo" is supposed to mean in this context, the original message was not clear either, but it seems that the "latter" that is rewritten automatically should be to method syntax, rather than "rewrite as foo".

Edit: ok I just realised that it meant "surround foo by backticks" - maybe this can be more explicit, because I interpreted it as "do nothing" - does not help that the other backticks in this error message just mean "code literal"

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that backticks are the hardest problem in modern computer science. The reason we don't hear this opinion expressed on the forums is that it is inexpressible because it requires backticks to say it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see where that may be confusing. I pushed a change to try to improve the situation.

@griggt griggt force-pushed the standardize-rewrite-ads branch from 62b2138 to 7b6b957 Compare May 10, 2022 02:51
i"""Alphanumeric $kind $name is not declared `infix`; it should not be used as infix operator.
|The operation can be rewritten automatically to `$name` under -deprecation -rewrite.
|Or rewrite to ${alternative(name)} manually.""",
i"""Alphanumeric $kind $name is not declared ${hl("infix")}; it should not be used as infix operator.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the syntax highlighting on infix has no effect, presumably because it is a soft keyword.

Copy link
Member

@bishabosha bishabosha May 10, 2022

Choose a reason for hiding this comment

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

could add some more utilities like:

def hlAsKeyword(str: String)(using Context): String = 
  if (str.isEmpty || ctx.settings.color.value == "never") str
  else s"${SyntaxHighlighting.KeywordColor}$str${SyntaxHighlighting.NoColor}"

@griggt griggt force-pushed the standardize-rewrite-ads branch from 7b6b957 to 97a20c7 Compare May 10, 2022 03:34
@griggt griggt force-pushed the standardize-rewrite-ads branch from 97a20c7 to b4043e3 Compare May 10, 2022 22:53
@griggt griggt requested a review from bishabosha May 10, 2022 23:02
@bishabosha bishabosha enabled auto-merge May 10, 2022 23:15
@bishabosha bishabosha merged commit 7c446ce into scala:main May 11, 2022
@griggt griggt deleted the standardize-rewrite-ads branch May 11, 2022 00:07
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
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.

6 participants