-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
v3 proposal #17
Comments
@snapshotpl
Each individual change should be one commit. Please create separate commits for each problem. Thank you in advance! Originally posted by @froschdesign at zendframework/zend-validator#177 (comment) |
@snapshotpl Totally missed this when I submitted #181 earlier. @froschdesign For large refactors, we simply cannot do one change at a time. In such cases, I would recommend posting a WIP with some of the ideas codified, and then creating an RFC in the contributors section of the forums. I'll be posting one related to #181 tomorrow, as I've done some significant work trying to address forwards and backwards compatibility at this point. Originally posted by @weierophinney at zendframework/zend-validator#177 (comment) |
You can, because a problem can change more than one line or one file! Originally posted by @froschdesign at zendframework/zend-validator#177 (comment) |
I suppose it all depends on what a "change" is. In this PR, there's a single commit that does two different things:
It seems to me that these should be two separate PRs. I agree with Matthew that more explanation on the reasons behind a significant change would be useful. Originally posted by @akrabat at zendframework/zend-validator#177 (comment) |
Replace the word "change" with "problem".
Right!
I agree also. I wanted to point out the problem with the "all-in-one-commits". No more. Originally posted by @froschdesign at zendframework/zend-validator#177 (comment) |
It extends #24 and add php 7.1 support
ping @weierophinney @Maks3w @bakura10
Originally posted by @snapshotpl at zendframework/zend-validator#177
The text was updated successfully, but these errors were encountered: