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

Bug Fix: Avoid over-deletion when looking for associated comma #573

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

ketkarameya
Copy link
Collaborator

@ketkarameya ketkarameya commented Aug 17, 2023

Previously, we were walking up the parent to find the associated trailing/leading comma.
But we did not have any safety checks guarding this comma deletions (like we have for comments).

Lets say in the below scenario we delete isFlgEnabled := exp.BoolValue(staleFlagConst) :

 t.Run("message", func(t *Foobar.T) {
         isFlgEnabled := exp.BoolValue(staleFlagConst)
         fmt.Println("some logging statement")
     })

Our comment/comma cleanup logic would kick in and cleanup stuff like below:

 t.Run("message"
         fmt.Println("some logging statement")
     })

Note we deleted the extra , func(t *Foobar.T) {

I introduced a simple safety check that ensures that there is no node between the comma and the node intended to be deleted.

Copy link
Contributor

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

Fix makes sense to me, but see comment about potentially missing test case. If there isn't any case that would fit, then let me know and I'll approve as is :)

t.Run1(func(t *Foobar.T) {
isFlgEnabled1 := exp.BoolValue(staleFlagConst)
fmt.Println("some other logging statement")
}, "other_message" )
Copy link
Contributor

Choose a reason for hiding this comment

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

In both these examples, the comma is before the deleted node, but the change handles both before and after, right? Any test case case you can think of for the "after" case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

aah I see ur point. I actually had a test case like that, but I had it slightly wrong. I reordered the statements here.
In our earlier logic we would delete }, in addition to isFlgEnabled1 := exp.BoolValue(staleFlagConst)

But now we don't, because we see that there is a node which ends between the ending of deleted node and comma

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but this change makes the comment above outdated, not?

// Should not delete this comment and the func below

It should now say something about the trailing },

Copy link
Collaborator Author

@ketkarameya ketkarameya Aug 22, 2023

Choose a reason for hiding this comment

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

good catch. Addressed. 0c68a1f

Copy link
Collaborator Author

@ketkarameya ketkarameya left a comment

Choose a reason for hiding this comment

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

addressed comments

Copy link
Contributor

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

👍

@ketkarameya ketkarameya merged commit 9538ce4 into master Aug 22, 2023
7 checks passed
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.

2 participants