-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 comparision for new model/body #4631
Conversation
@@ -478,42 +478,26 @@ def check_macros_modified(self, node): | |||
previous_macros = [] | |||
return self.recursively_check_macros_modified(node, previous_macros) | |||
|
|||
def check_modified(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: | |||
# TODO check modifed_content and check_modified macro seems a bit redundent |
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.
@gshank do you know what's the main difference between these two functions? seems a little bit confusing here
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.
I think the way you're doing this is fine. The check_modified
method (state:modified
for users) is meant to be the superset of all checks. Renaming this so it clearly maps to same_contents
(the umbrella check) makes sense
'modified': | ||
self.check_modified_content, | ||
'modified.body': | ||
self.check_modified_factory('same_body'), |
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.
Let me know if you think we should go change the underlying function names instead of using the factory here! they are used somewhere but I think it is not too much
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.
I like the factory. It avoids duplicating the code in the functions.
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.
Looks good! I like the improved efficiency.
'modified': | ||
self.check_modified_content, | ||
'modified.body': | ||
self.check_modified_factory('same_body'), |
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.
I like the factory. It avoids duplicating the code in the functions.
@ChenyuLInx this PR needs backported if it hasn't yet |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.0.latest 1.0.latest
# Navigate to the new working tree
cd .worktrees/backport-1.0.latest
# Create a new branch
git switch --create backport-4631-to-1.0.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 e6786a2bc31a5b22e01a90eea36263a1afb4fce8
# Push it to GitHub
git push --set-upstream origin backport-4631-to-1.0.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.0.latest Then, create a pull request where the |
* fix comparison for new model/body
* fix comparison for new model/body automatic commit by git-black, original commits: e6786a2
resolves #4570
Description
Selecting modified.body used to throw an error after adding a new model. This change fixed the behavior to return the newly added model boday
Checklist
CHANGELOG.md
and added information about my change