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

No unit insertion as an addition of no auto tupling #352

Merged
merged 2 commits into from
Sep 17, 2017

Conversation

insdami
Copy link
Contributor

@insdami insdami commented Sep 15, 2017

This PR is for #214.
I'm not quite sure how to compose it with NoAutoTupling to run it using NoAdaptedArgs as you propose on the comments.

Have a look at it and let me know, please :)

@jvican jvican added the spree label Sep 15, 2017
Copy link

@jvican jvican left a comment

Choose a reason for hiding this comment

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

Excellent work @insdami! Thank you for sharing our interest in this cool tool. The patch LGTM.

I hand the rest of the review to @olafurpg.

/cc @gabro who asked for this feature.

@gabro
Copy link
Collaborator

gabro commented Sep 15, 2017

I'll take a better look from a computer, but it looks 👌

I'm thinking we can probably merge this into NoAutoTupling and just rename the whole thing NoArgumentListAdaptation or something along those lines.

WDYT @olafurpg?

@gabro
Copy link
Collaborator

gabro commented Sep 15, 2017

Also many thanks @insdami for picking this up, it's a really nice addition to Scalafix!

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @insdami ! I am hiking this weekend at Scala World so my responses may be slow :)

I agree with @gabro, can we merge this with NoAutoTupling? We can keep the test suite separate, however. We may want to consider renaming the rewrite to NoAdaptedArgs eventually, but that can be left for a future PR.

Otherwise this PR looks great!

ctx.addRight(t.tokens.init.last, "()")

override def fix(ctx: RuleCtx): Patch = {
val adaptations = index.messages.toIterator.collect {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will iterate through all reported messages in the workspace/project on every source file, we should move it outside of the fix method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll do the same in NoAutoTupling

Copy link
Collaborator

@gabro gabro left a comment

Choose a reason for hiding this comment

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

Looking 💯

Copy link
Contributor

@olafurpg olafurpg 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!

@olafurpg olafurpg merged commit 06edc24 into scalacenter:master Sep 17, 2017
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