-
Notifications
You must be signed in to change notification settings - Fork 286
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
feat: --find-renames for discovering renames based on content #320
Conversation
I do not see a way around the O(n^2) runtime, except with some heuristics. But I think in most use-cases that is acceptable.
|
fcf340c
to
3537cb4
Compare
The test case contains blank lines with white-spaces and many editors do trim those, so we adopt the test as well as the code to not indent blank lines
The three steps are called in three places, we better combine them into a single function.
By encapsulating the parameters into a struct and a corresponding cmd.options package helper functions, we need only to change the code in two places when adding a new parameter instead of four (for each subcommand).
If we rename objects, they will appear as add and remove actions, which makes it difficult to assess the semantic delta, which may only be a simple renaming. The current implementation creates a diff between every added & removeed item of the same kind, which means at worst O(m*n) runtime. So it should only be enabled if such changes are rare
When implementing the tests, I was running into some minor issues I couldn't explain, so I split up the changes into several commits and refactored the code to limit the change required to implement the functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks a lot for your efforts!
If we rename objects, they will appear as
add and remove actions, which makes it difficult
to assess the semantic delta, which may only be
a simple renaming.
The current implementation creates potentially
a diff between every add/remove item, which
means O(n^2) runtime. So it should only be
enabled if such changes are rare