Skip to content
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

added allNullOr* and nullOrAll* methods documentation and tests #68

Closed
wants to merge 5 commits into from
Closed

added allNullOr* and nullOrAll* methods documentation and tests #68

wants to merge 5 commits into from

Conversation

rvitaliy
Copy link

@rvitaliy rvitaliy commented May 2, 2018

currently this libs allow call methods like:

Assert::allNullOrString([null, 'string']);
or
Assert::nullOrAllString(null);
Assert::nullOrAllString(['str1', 'str2']);

this PR add test coverage and method documentation to all this methods.

if community approve this PR, I provide to update README.mb and CHANGELOG.mb

Copy link
Contributor

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR.

It is a bit hard to review with the major diff. And some changes is is just style fixes.

Could you maybe explain why you are doing these 2? 3? different changes and then make them in separate PRs?

tests/AssertTest.php Outdated Show resolved Hide resolved
src/Assert.php Outdated Show resolved Hide resolved
tests/AssertTest.php Outdated Show resolved Hide resolved
@rvitaliy
Copy link
Author

@Nyholm ok, i'll split this PR in 2 or 3 punctual PR to make changes clearer

@rvitaliy
Copy link
Author

@Nyholm after some investigation i preferred to split in more commits instead of more PR.
let me know if now it is clearer.
I reverted alphabetical order of method documentation to avoid conflicts.

Copy link
Contributor

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good.
The diff is quite unfair to you. I had some smaller issues. I will do a final check when they are ready.

tests/AssertTest.php Outdated Show resolved Hide resolved
tests/AssertTest.php Outdated Show resolved Hide resolved
tests/AssertTest.php Outdated Show resolved Hide resolved
@rvitaliy
Copy link
Author

rvitaliy commented Jun 7, 2018

@Nyholm [need-help] can't understand why AppVeyor build failed

@Nyholm
Copy link
Contributor

Nyholm commented Jun 28, 2018

I think AppVeyor is unrelated.

@rvitaliy rvitaliy closed this Jan 28, 2020
@rvitaliy rvitaliy deleted the feature/added-allNullOr-nullOrAll-merhod-documentantion-and-tests branch January 28, 2020 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants