Skip to content

Extend array assertions - #18

Closed
Zales0123 wants to merge 2 commits into
webmozarts:masterfrom
Zales0123:count-assert-function
Closed

Extend array assertions#18
Zales0123 wants to merge 2 commits into
webmozarts:masterfrom
Zales0123:count-assert-function

Conversation

@Zales0123

Copy link
Copy Markdown
Contributor

Hello everybody!

First of all, I'd like to thank you for the amazing job with this library, it really fits my needs, I get used to use it and don't think I would like to change it at all 😄

As my contribution for this library development, I would propose introducing some more assertions associated with arrays. Starting from the simple Assert::count(...) -> from my experience it would be pretty useful, as now to check array length, you must eq with count($array) which just looks bad 🐃

Let me know is it something that could be useful for this library. Thank you for feedback and see ya in code ;)

@Spomky

Spomky commented Oct 1, 2016

Copy link
Copy Markdown

Could you add also inArray assertion?

@webmozart webmozart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, sounds useful to me! :)

Can you please add the corresponding @method lines in the PhpDoc of the class? (look at the existing ones for example)

As for inArray(), there is oneOf() which fulfills this purpose.

Comment thread README.md Outdated
`keyExists($array, $key, $message = '')` | Check that a key exists in an array
`keyNotExists($array, $key, $message = '')` | Check that a key does not exist in an array
`keyNotExists($array, $key, $message = '')` | Check that a key does not exist in an
`count($array, $number, $message = '')` | Check that an array contains specific number of elements

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Grammar: "contains a specific number of elements"

Comment thread src/Assert.php Outdated
static::eq(
count($array),
$number,
$message ?: sprintf('Array should have %d elements, but has %d.', $number, count($array))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consistency with the other exception messages, I'd change this to:

Expected an array to contain %d elements. Got: %d

@Zales0123
Zales0123 force-pushed the count-assert-function branch from 6842a83 to 625049b Compare October 10, 2016 07:47
@Zales0123
Zales0123 force-pushed the count-assert-function branch from 625049b to 2126e92 Compare October 10, 2016 07:48
@Zales0123

Copy link
Copy Markdown
Contributor Author

@webmozart fixed, hope it is perfect now ;)

Comment thread README.md Outdated
------------------------------------------- | --------------------------------------------------
`keyExists($array, $key, $message = '')` | Check that a key exists in an array
`keyNotExists($array, $key, $message = '')` | Check that a key does not exist in an array
`keyNotExists($array, $key, $message = '')` | Check that a key does not exist in an

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Check that a key does not exist in an array"

You are missing "array"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

:( gonna fix it asap

@Zales0123
Zales0123 force-pushed the count-assert-function branch from 2126e92 to 0c782e0 Compare October 13, 2016 20:40
@Zales0123

Copy link
Copy Markdown
Contributor Author

@webmozart sorry for the delay with last fixes (in fact, I was 100% sure I pushed them few days ago 😄 ). Hope really everything is now good and mergeable 👍

webmozart added a commit that referenced this pull request Oct 17, 2016
Count assert function. Will replace #18
@webmozart

Copy link
Copy Markdown
Member

Replaced by #24

@webmozart webmozart closed this Oct 17, 2016
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.

4 participants