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

AddTimeUnitArgument recipe #403

Merged
merged 11 commits into from
Aug 2, 2023
Merged

AddTimeUnitArgument recipe #403

merged 11 commits into from
Aug 2, 2023

Conversation

joanvr
Copy link
Contributor

@joanvr joanvr commented Aug 2, 2023

What's changed?

Added a new recipe AddTimeUnitArgument to solve the timeout and timevalue migrations. This way is more maintainable since it only requires changes in the declarative yml recipe instead of changing the recipe themselves.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@joanvr joanvr requested review from timtebeek and rpau August 2, 2023 08:28
@timtebeek timtebeek added the recipe Recipe requested label Aug 2, 2023
Joan Viladrosa and others added 8 commits August 2, 2023 12:02
Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
…nitArgument.java

Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
…nitArgument.java

Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
…nitArgument.java

Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
…nitArgument.java

Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
…nitArgument.java

Co-authored-by: Raquel Pau <1483433+rpau@users.noreply.github.com>
@joanvr joanvr requested review from rpau and timtebeek August 2, 2023 10:29
Comment on lines +88 to +89
updateCursor(m),
m.getCoordinates().replaceArguments(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering what happens to the method type when we don't replace the method, but only add an argument; wouldn't the method type post-recipe still refer to the original (usually single argument) method instead of the new multiple argument method? I'd always thought that replaceArguments was only when you replace with the same number and type or arguments; not when you switch to a different method.

If the method type is updated correctly there's no need for a change, otherwise it might be best to replace the whole method rather than just the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the method type is updated correctly

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Already approved, but do look at my last question regarding replace arguments versus replace method and the resulting method type.

@joanvr joanvr merged commit 3c42c95 into main Aug 2, 2023
1 check passed
@joanvr joanvr deleted the httpclient/timeunit branch August 2, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants