v3 proposal - Githubissues

zendframework / zend-validator

Validator component from Zend Framework

BSD 3-Clause "New" or "Revised" License

181 stars 136 forks source link

v3 proposal #177

Open snapshotpl opened 7 years ago

snapshotpl commented 7 years ago

It extends #24 and add php 7.1 support

ping @weierophinney @Maks3w @bakura10

froschdesign commented 7 years ago

@snapshotpl I see some problems, because you've mixed different problems in one commit:

this breaks the version control system
no way for a cherry-pick
reading the commit history is a mess
a review is very difficult

Each individual change should be one commit. Please create separate commits for each problem. Thank you in advance!

weierophinney commented 7 years ago

@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.

froschdesign commented 7 years ago

@weierophinney

For large refactors, we simply cannot do one change at a time.

You can, because a problem can change more than one line or one file!

akrabat commented 7 years ago

I suppose it all depends on what a "change" is.

In this PR, there's a single commit that does two different things:

Updates travis.yml and composer.json to PHP 7.1
Adds Result.php, Message.php and the changes ValidatorInterface.php.

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.

froschdesign commented 7 years ago

@akrabat

I suppose it all depends on what a "change" is.

Replace the word "change" with "problem".

In this PR, there's a single commit that does two different things

Right!

I agree with Matthew that more explanation on the reasons behind a significant change would be useful.

I agree also.

I wanted to point out the problem with the "all-in-one-commits". No more. We should add a new paragraph to the contributors guide to respect the principles of a version control system.

weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-validator; a new issue has been opened at https://github.com/laminas/laminas-validator/issues/17.

weierophinney commented 4 years ago

This repository has been moved to laminas/laminas-validator. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-validator to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-validator.
In your clone of laminas/laminas-validator, commit the files, push to your fork, and open the new PR. We will be providing tooling via laminas/laminas-migration soon to help automate the process.