Consolidate diff parsing logic #125
Conversation
|
Heads up! This PR modifies the following files:
|
|
|
|
The "unsafe" warning seems to triggered by the changes to |
| return set(f if f.startswith('/') else f[2:] for f in changed_files) | ||
| return set(f for f in map(self.normalize_file_path, changed_files) if f is not None) | ||
|
|
||
| def normalize_file_path(self, filepath): |
There was a problem hiding this comment.
Let's make this a standalone function rather than a method.
|
|
|
Thank you for doing this work, @numberMumbler! The resulting code is much nicer to reason about :) |
|
Are you planning to finish this work, @numberMumbler? |
|
yes, I should have the updates finished up this weekend |
|
Great; thanks! |
|
☔ The latest upstream changes (presumably #138) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@numberMumbler Any updates on this? |
|
New code was committed to pull request. |
|
New code was committed to pull request. |
|
New code was committed to pull request. |
|
@wafflespeanut Should be ready to go |
|
I think there are a few commits that could be squashed, but honestly, I've never been so detailed in my commits. So, I'll leave this to @jdm :) |
|
@wafflespeanut In these situations, is it better to just re-do the changes (in fewer commits)? I can consolidate a lot of these changes… which I'm sure would make it easier to review |
|
I was fine with reviewing each change in isolation. This looks good to me! |
|
Thanks for doing this work, @numberMumbler! |
Removing unused parameter
Moves diff header parsing logic out of the handlers and into the
APIProviderclass. This allows the code to be reused, reduces the amount of code to maintain, and simplifies the handlers' logicFixes #47