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

Remove all references to unsupported rewrite rules #2929

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

johnduffell
Copy link
Contributor

@johnduffell johnduffell commented Nov 23, 2023

I spent a while trying to get the documented rule to work, but the release versions of the rule were confusing and the rules hadn't been built by any recent version of scalafix and the older versions didn't work with the latest scala 2.13 versions. It looked like they were knocked up to get a specific migration done and then unmaintained.

I didn't have any luck trying to build them myself, then once I gave up I found a ready made built in scalafix rule that did exactly what I needed!

This PR updates the docs accordingly.

I also had a search through and found a few more outdated references to update.

@johnduffell johnduffell changed the title point to working built in scalafix rule Remove all references to unsupported rewrite rules Nov 23, 2023
@@ -337,11 +337,11 @@ This command runs a number of Scalafix rules to patch some discarded syntax.

The list of applied Scalafix rules are:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@som-snytt
Copy link
Contributor

This LGTM but I'm not familiar with the scalafix ecosystem. It is weird to have links to unofficial code, in the context of migration tooling which I just saw advertised in a video.

The last link (the improvement) is possibly extraneous. "When in doubt, omit," as I just read in Tolstoy.

Probably it's worth squashing the commits for improved lines/commit.

I'll defer to @SethTisue to hit the green button.

@SethTisue
Copy link
Member

@adpi2 is this something you would like to review, or do you know of an appropriate person?

if not, let's optimistically merge before too much longer

@som-snytt
Copy link
Contributor

I'm able to merge pessimistically. ("It couldn't be any worse than...")

@johnduffell
Copy link
Contributor Author

OK I've managed to merge and squash thanks to stackoverflow so it should be as clean as it ever will be!
I agree the changes themselves better even if not perfect.

@som-snytt
Copy link
Contributor

It seems that the overview for old scala.reflect points to the current api URL newly taken over by Scala 3, if I skimmed correctly. Deciding now whether to visit Costco before closing, not sure if that is just config somewhere.

Copy link
Member

@adpi2 adpi2 left a comment

Choose a reason for hiding this comment

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

Thanks! This is indeed a more honest view of the current state of our Scalafix rules.

@adpi2 adpi2 merged commit 02acc36 into scala:main Dec 14, 2023
1 check failed
@johnduffell johnduffell deleted the patch-1 branch December 14, 2023 16:17
@SethTisue
Copy link
Member

Thank you @johnduffell! Very glad to have these changes. Sorry we were a bit pokey about responding.

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.

4 participants