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

IsNotEmpty operation #292

Merged
merged 5 commits into from
Jan 13, 2023
Merged

IsNotEmpty operation #292

merged 5 commits into from
Jan 13, 2023

Conversation

jazithedev
Copy link
Contributor

@jazithedev jazithedev commented Jan 4, 2023

This PR:

  • Provide a new IsNotEmpty operation
  • Is covered by unit tests
  • Has static analysis tests (psalm, phpstan)
  • Has documentation

Related to #291.

@jazithedev jazithedev requested a review from drupol as a code owner January 4, 2023 21:33
src/Operation/IsNotEmpty.php Outdated Show resolved Hide resolved
docs/pages/api.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the PR, everything seems ok, I don't have much to say.

However, there's one missing thing.

Since we include a CollectionDecorator, you need to add the new method in it as well.

Once that issue and the other minor inline comment are done, I'll wait 24h before merging it, so @AlexandruGG is able to review it as well.

Once again, thanks for taking the time to do that !

PS: Do not mind the static analysis issues with PSalm, we are working on it in #287

@drupol
Copy link
Collaborator

drupol commented Jan 7, 2023

@ktrzos Are you at ease with the requested changes? Feel free to ask more info if needed!

@jazithedev
Copy link
Contributor Author

@drupol

Sorry. Didn't have much time during the last days. But things are fixed now 🙂.

@drupol
Copy link
Collaborator

drupol commented Jan 8, 2023

It's super fine, no worries.

Thanks for fixing it !

AlexandruGG
AlexandruGG previously approved these changes Jan 8, 2023
Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! 👍

A couple of minor comments - I'll approve but please update the docs accordingly.

src/Contract/Operation/IsNotEmptyable.php Outdated Show resolved Hide resolved
docs/pages/api.rst Show resolved Hide resolved
@jazithedev
Copy link
Contributor Author

@AlexandruGG
Good catch! Updated 👍️.

@drupol drupol merged commit 09ad315 into loophp:master Jan 13, 2023
@drupol
Copy link
Collaborator

drupol commented Jan 13, 2023

Thanks !!!

I hope it was not too complex! Feel free to come up with new additions!

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.

3 participants