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

fix: remove suggestions' duplication #286

Merged
merged 8 commits into from
Jul 18, 2023
Merged

Conversation

granddaifuku
Copy link
Contributor

@granddaifuku granddaifuku commented Nov 8, 2022

What does this PR accomplish?

  • 🩹 Bug Fix

Closes #10 .

Changes proposed by this PR:

To remove the duplicated suggestions, the Vector of Suggestion is converted once to a HashSet and then again to a Vector.

Notes to reviewer:

This PR tries to resolve the 🔰 part of the issue, but I'm willing to work on further parts of it.

📜 Checklist

  • Works on the ./demo sub directory
  • Test coverage is excellent and passes
  • Documentation is thorough

@granddaifuku granddaifuku changed the title feat: remove suggestions' duplication fix: remove suggestions' duplication Nov 8, 2022
@granddaifuku granddaifuku marked this pull request as draft November 8, 2022 16:44
@drahnr
Copy link
Owner

drahnr commented Nov 9, 2022

This practically solves the problem of identical suggestions, but how about partial overlaps? Do you have an idea (theoretically) how to deal with multiple overlapping suggestions, at least avoiding corruption?

@granddaifuku
Copy link
Contributor Author

Thank you for your reviews!

I'm still striving to find a better solution for the case there are multiple mistakes.

Could I make sure my understanding is that

multiple mistakes that are overlapping and/or with different fix suggestions.

means Where checkers point out are same, but the way they suggest is different?

If so, I think we could achieve this by iterating through both resulted collections and merging the overlapped ones based on the fields of Suggestion: origin, span, and range. (Here, in this case, we don't need to use HashSet.)
I'll appreciate it if you could give me feedback on this!

@drahnr
Copy link
Owner

drahnr commented Nov 9, 2022

The difficulty is overlapping changes that are confliciting, or even if they're not, how to merge them.

My idea would be, to omit one of them (the latter one), and re-run on the relevant chunk after applying the first suggestion.

@granddaifuku
Copy link
Contributor Author

granddaifuku commented Nov 9, 2022

Thank you for your help!

It makes sense!!

So, in that case, we need to identify overlapping changes, then apply the suggestion internally on the firstly overlapped one, and re-run the checker on the latter ones.
Does this match what you are thinking?

@drahnr
Copy link
Owner

drahnr commented Nov 9, 2022

Yes, that's precisely it. Sorting might help and then iterating over adjacent tuples, just an idea. I don't have a vision on how the checker would be re-run, though. In a perfect world, without any additional IO.

@granddaifuku
Copy link
Contributor Author

Great thanks!

Maybe it will take a few days, but I will give it a try.

@granddaifuku
Copy link
Contributor Author

Sorry for the slow progress.
I have just implemented a method to determine the overlappings. After this, I will implement the method to determine and process the checker's overlappings.

@drahnr
Copy link
Owner

drahnr commented Jan 13, 2023

Hey @granddaifuku 👋 - gentle ping in the new year 🎆 - is there anything you're blocked on I could help with?

@granddaifuku
Copy link
Contributor Author

granddaifuku commented Jan 13, 2023

Happy new year @drahnr ! Thank you for caring about this!!

I'm sorry I couldn't make any progress on this PR due to my thesis.
My thesis defense is at the beginning of February, so I can restart working on this around the mid of February.

@drahnr
Copy link
Owner

drahnr commented Jan 19, 2023

Best of luck (which I am sure you don't need) @granddaifuku ! Feel free to ping me once you're done :)

@drahnr
Copy link
Owner

drahnr commented Feb 12, 2023

Gentle ping, now that people are starting to hit issues with this i.e. #295

@granddaifuku
Copy link
Contributor Author

@drahnr
Thank you for reaching out to me multiple times.
I would like you to know that my thesis defense is now over, so I will restart working on this PR this Sunday!

@granddaifuku
Copy link
Contributor Author

@drahnr
I made some updates that detect overlapped suggestions.
However, I still could not find a way to re-run the checker after internally applying the first suggestion.
Could you help me with that point?

@drahnr
Copy link
Owner

drahnr commented Feb 22, 2023

Hey @granddaifuku I'll try to find some time later this week :) for a hunch, see #296

src/checker/mod.rs Outdated Show resolved Hide resolved
src/suggestion.rs Outdated Show resolved Hide resolved
Copy link
Owner

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Good start :) a few stylicstic and domain specific topics to address, regarding the re-running of the checker, it's a non-trivial undertaking and should possibly be deferred to separate PR.

@granddaifuku
Copy link
Contributor Author

@drahnr
Thank you for your reviews! I've addressed the stylistic point and will work on the second one as well. I will let you know once I've made the changes, so please review them when you have a chance. Thank you again.

src/checker/mod.rs Outdated Show resolved Hide resolved
Copy link
Owner

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

Generally looks good, one fix pending :)

@granddaifuku
Copy link
Contributor Author

@drahnr
Great, thank you for your review! I have made a change to remove any overlapping suggestions from the suggestions vector. I will also be making a change to the logic to be more accurate in detecting overlap and will update you once that is done.

@granddaifuku
Copy link
Contributor Author

@drahnr
I wanted to let you know that I made some changes to the is_overlapped method to check for overlaps in multi-line suggestions. Whenever you have some free time, I'd appreciate it if you could take a look and review the changes I made. If everything seems to be in order, I would like to add tests for the updated functionality.

src/suggestion.rs Outdated Show resolved Hide resolved
@drahnr
Copy link
Owner

drahnr commented Jul 3, 2023

@granddaifuku sorry for the long silence, is there anything left to do from your perspective? Did you test it as part of the dummy projects in the working tree?

@granddaifuku
Copy link
Contributor Author

granddaifuku commented Jul 4, 2023

@drahnr
Thank your for your response, and I'm sorry for not ping you again for a long time.
I already checked it with a demo project, and it worked.
I will make this PR ready for a review then.

@granddaifuku granddaifuku marked this pull request as ready for review July 4, 2023 13:41
@drahnr drahnr merged commit da9c125 into drahnr:master Jul 18, 2023
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.

merge exact matches of overlapping suggestion
2 participants