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

Refactor Scalar Operations #132

Merged
merged 16 commits into from
Jul 11, 2021
Merged

Refactor Scalar Operations #132

merged 16 commits into from
Jul 11, 2021

Conversation

AlexandruGG
Copy link
Collaborator

@AlexandruGG AlexandruGG commented Jul 9, 2021

This PR refactors Collection Operations which currently return a single scalar value inside a collection, returning the scalar directly.

  • Contains
  • Every
  • Falsy
  • Has
  • Match + static analysis check & type fixes
  • Nullsy
  • Truthy

Related to #107.

docs/pages/api.rst Outdated Show resolved Hide resolved
@AlexandruGG
Copy link
Collaborator Author

I'm gonna give this one a final look a bit later before marking it ready for review

@drupol
Copy link
Collaborator

drupol commented Jul 10, 2021

I'm gonna give this one a final look a bit later before marking it ready for review

Take your time mate, there is no hurry.

Do you think it would be possible to add some documentation that explains

  • All the methods of the Collection returns a new collection
  • Only the methods X,Y,Z returns something different
  • Explaining the reasons of this breaking change.

I will also use that part in the changelog when doing the release later on.

@AlexandruGG
Copy link
Collaborator Author

I'm gonna give this one a final look a bit later before marking it ready for review

Take your time mate, there is no hurry.

Do you think it would be possible to add some documentation that explains

  • All the methods of the Collection returns a new collection
  • Only the methods X,Y,Z returns something different
  • Explaining the reasons of this breaking change.

I will also use that part in the changelog when doing the release later on.

Yeah for sure! I was thinking to add a part in the documentation to explain about the operations but I forgot about it there for a moment 😄.

Regarding the last bullet point do you want that to be in the documentation or do you mean in the PR description?

@drupol
Copy link
Collaborator

drupol commented Jul 10, 2021

Ok good 👍

I think we can do it in the documentation and I'll copy paste it for the changelog later.

@AlexandruGG
Copy link
Collaborator Author

Ok good 👍

I think we can do it in the documentation and I'll copy paste it for the changelog later.

Sounds good. Btw for BC breaking changes like this should we have a new major version? I.e 5.0?

@drupol
Copy link
Collaborator

drupol commented Jul 10, 2021

I don't know yet. We will see when it's done.

@@ -76,7 +76,7 @@ This library has been inspired by:
needed are standard PHP variables like `int`, `string`, `callable` or
`iterator`.
It allows users to use those operations individually, at their own will, to
build up something custom. Currently, more than [**80 operations**][28] are
build up something custom. Currently, more than [**100 operations**][28] are
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did a new count and this is up over 100 now 🎉

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent :-)

@AlexandruGG
Copy link
Collaborator Author

AlexandruGG commented Jul 11, 2021

I don't know yet. We will see when it's done.

I think it depends if we want this project to follow semver or not. Because based on that any backwards-incompatible API changes should be done as part of a new major version.

Also if we do up the version when releasing this we should up the docs version as well.

@AlexandruGG AlexandruGG marked this pull request as ready for review July 11, 2021 10:36
@AlexandruGG
Copy link
Collaborator Author

@drupol let me know what you think about the documentation update

When used as a ``Collection`` method, operations fall into three main categories based on the return type:

1. Operations that return a ``scalar`` value. Currently, this includes:
``Contains``, ``Every``, ``Falsy``, ``Has``, ``Key``, ``Match`` (or ``MatchOne``), ``Nullsy``, ``Truthy``.
Copy link
Collaborator

@drupol drupol Jul 11, 2021

Choose a reason for hiding this comment

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

I think the explanation is overall really great and I wouldn't change a thing, except for the classification points here.

What I propose is to list the operation types as such:

  1. Operations which returns a boolean: contains, every, falsy, has... etc etc
  2. Operations which returns a Collection of Collection: span, partition
  3. Operation which returns an array: all
  4. Operation which returns a Collection: all the rest.

(be carefull Collection::key() might return anything, not only a scalar, but anything)

Also, what do you think about:

  • Updating the signatures of all the operations in the API page and add their return type ?
    or
  • Updating all operations and add a note about the return type ?

(about that page, it's very sad we cannot generate the documentation automatically from the classes themselves!)

Let me know what you think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drupol thank you for the feedback, I've done some changes to the numbered list to incorporate your suggestions, with a couple tweaks. Let me know what you think of this new classification :).

Regarding updating the signatures of the operations in the docs -> it's something that I thought about today as well 😄 . I'll do it in a future PR for all of them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks all good now :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drupol great, this is all ready on my side 😁

@drupol
Copy link
Collaborator

drupol commented Jul 11, 2021

I don't know yet. We will see when it's done.

I think it depends if we want this project to follow semver or not. Because based on that any backwards-incompatible API changes should be done as part of a new major version.

Also if we do up the version when releasing this we should up the docs version as well.

We should definitely follow semver... and therefore it's going to be version 5 indeed.

@AlexandruGG
Copy link
Collaborator Author

I don't know yet. We will see when it's done.

I think it depends if we want this project to follow semver or not. Because based on that any backwards-incompatible API changes should be done as part of a new major version.
Also if we do up the version when releasing this we should up the docs version as well.

We should definitely follow semver... and therefore it's going to be version 5 indeed.

Cool in that case I think we can do the map change as well to remove the deprecation and only allow one callback, I'll do it in a future PR.

@drupol drupol merged commit 65d1758 into master Jul 11, 2021
@drupol drupol deleted the feature/scalar-operations branch July 11, 2021 18:10
@drupol
Copy link
Collaborator

drupol commented Jul 11, 2021

Thanks, a big one done 🥇

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

Successfully merging this pull request may close these issues.

None yet

2 participants