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

Further enhance 0.5 migration scalafix #237

Merged
merged 5 commits into from
Feb 1, 2024
Merged

Further enhance 0.5 migration scalafix #237

merged 5 commits into from
Feb 1, 2024

Conversation

bpholt
Copy link
Member

@bpholt bpholt commented Jan 19, 2024

I think this covers the migration cases I was looking for, with the exception that I haven't been able to figure out how to globally replace tagChunkSize with ChunkSize and also have it work inline in a encrypt method call without publishing two separate rules.

Unless anyone has any great ideas, on Monday I'll probably start working on a second rule and documentation stating "run the tagChunkSizeChunkSize rule first" and then the more universal rule second.

@bpholt
Copy link
Member Author

bpholt commented Jan 19, 2024

@CJSmith-0141 Do you have any suggestions here?

@bpholt bpholt self-assigned this Jan 19, 2024
@CJSmith-0141
Copy link
Contributor

The advice from this post by @/bjaglin on the scalameta discord is:

You should always aim at patching the smallest tree possible

So I think that you're on the right track with your commented out code here

Maybe a strategy like you did with migrateEncrypt ? Write a private function that does the tagChunkSize migration and invoke it in two places

  • at the top level but with a predicate to ensure that match isn't a child of any encrypt using Tree.parent
  • another time in migrateEncrypt

@CJSmith-0141
Copy link
Contributor

I took a shot at it in #238 targeting this branch.

@bpholt bpholt marked this pull request as ready for review January 30, 2024 00:58
@bpholt bpholt requested a review from a team as a code owner January 30, 2024 00:58
@bpholt
Copy link
Member Author

bpholt commented Jan 30, 2024

@CJSmith-0141 I incorporated your suggestion from #238 and took it a step further. I think this is pretty close to being ready to merge and release; what do you think?

Copy link
Contributor

@CJSmith-0141 CJSmith-0141 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for putting time into this @bpholt

@bpholt bpholt merged commit d171fda into series/0.5 Feb 1, 2024
15 checks passed
@bpholt bpholt deleted the scalafixes branch February 1, 2024 16:23
@bpholt bpholt added this to the v0.5 milestone Feb 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