-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
chore: Allow only iterables for BaseDAO.delete() #25844
chore: Allow only iterables for BaseDAO.delete() #25844
Conversation
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
@john-bodley I like the cleanup but isn't this a breaking change? What if someone is extending the |
@michael-s-molina couldn’t you extend that same logic for any change, i.e., in #25819 what if someone was referring to the old class? I guess it really depends on one’s interpretation of what’s public and in this case whether the intent is for DAOs to extended/leveraged in custom configurations. |
The difference in this case is that tags are not officially released yet.
I agree. Some parts are easy to identify as potential for breaking changes such as APIs or database engine specs. Other parts such as DAOs or form data are not so clear. Maybe we should bring this discussion to the Town Hall and formalize the constraints. @villebro @rusackas @betodealmeida @eschutho |
@michael-s-molina per the recently added Semantic Versioning wiki page do you feel this is non-breaking and thus could be merged prior to the 3.1 branch cut? |
SUMMARY
Previous—due to legacy reasons when we had both single and bulk delete logic—the
BaseDAO.delete()
method accepted either a single item or a list of items. For simplicity and clarity this PR updates the function signature to support only the later. i.e., it is now apparent from the code that deletion is a bulk operation.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION