Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

v3 proposal #177

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

v3 proposal #177

wants to merge 1 commit into from

Conversation

snapshotpl
Copy link

It extends #24 and add php 7.1 support

ping @weierophinney @Maks3w @bakura10

@snapshotpl snapshotpl changed the base branch from master to develop June 27, 2017 21:47
@froschdesign
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor

akrabat commented Jul 19, 2017

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
Copy link
Member

@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
Copy link
Member

This repository has been closed and moved to laminas/laminas-validator; a new issue has been opened at laminas/laminas-validator#17.

@weierophinney
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants