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

Introduce helpers for static constructors #158

Closed
wants to merge 2 commits into from

Conversation

AlexandruGG
Copy link
Collaborator

@AlexandruGG AlexandruGG commented Jul 24, 2021

This PR provides helper functions that can be used to instantiate Collection.
The main advantages are:

  • fewer characters to type
  • less import aliasing, e.g. when using the CollectionInterface as param or return type users always need to have 2 imports with the existing static constructors - one for the interface and one for the Collection class (see discussion link below)
  • Provides helper functions
  • Has unit tests (phpspec)
  • Has static analysis tests (psalm, phpstan)
  • Has documentation

Related to #135 (comment).

Comment on lines +41 to +46
public function getMatchers(): array
{
return [
'beSameAs' => static fn (CollectionInterface $subject, CollectionInterface $expected): bool => $subject->same($expected),
];
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding a custom matcher based on: https://www.phpspec.net/en/stable/cookbook/matchers.html#inline-matcher

Couldn't find a better way to test the function, let me know if you can think of any

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good way indeed! And I don't see any other option at the moment.

@AlexandruGG
Copy link
Collaborator Author

@drupol does this look good to you? If so, I can add functions for the other constructors

@drupol
Copy link
Collaborator

drupol commented Jul 24, 2021

Hi Alex,

Static constructors are already helpers and I don't like adding this so much.

Even after reading the description, I'm still having troubles seeing the advantages of this.

The documentation is hopefully written correctly now to explain how to create Collection, why would we want to have a function that does it on top of what's already existing?

@AlexandruGG
Copy link
Collaborator Author

Hi Alex,

Static constructors are already helpers and I don't like adding this so much.

Even after reading the description, I'm still having troubles seeing the advantages of this.

The documentation is hopefully written correctly now to explain how to create Collection, why would we want to have a function that does it on top of what's already existing?

Hey @drupol,

Thanks for the feedback. I had a few conflicting thoughts about this as well, but I still think I would find it useful.

I think it would be great if we could list both the pros and cons of this. These are the ones I can think of:

Pros:

  • no need for two separate CollectionInterface and Collection imports (more details in the discussion)
  • less characters to write. Might not seem like a big one, but if you you're writing many Collection::fromIterable vs collectIterable it can make a difference

Cons:

  • "duplicating" functionality of the existing static constructors -> since the functions would be calling the static constructors anyway, the code is not really duplicated
  • global functions might clash with other user-defined functions -> since we use function_exists, in cases where collectIterable already exists it just wouldn't be defined. Those users would have to choose if they want to change their own function or just use the static constructors
  • using global functions in general -> these functions are namespaced so an import would always be needed

We could argue that the pros are not that significant and I partly agree, but would be nice to hear your reasoning.

@drupol
Copy link
Collaborator

drupol commented Jul 25, 2021

Hello there,

Pros:

* no need for two separate `CollectionInterface` and `Collection` imports (more details in the [discussion](https://github.com/loophp/collection/discussions/135#discussioncomment-1016598))

But why would this be a pro? I don't see any issue of having 1 or 2 use statements in the header of the file. I never find it problematic.

* less characters to write. Might not seem like a big one, but if you you're writing many `Collection::fromIterable` vs `collectIterable` it can make a difference

Yeah... but we have autocompletion and I never write it completely, but rely on my IDE to autocomplete it. So here, I don't see the difference between the 2 options.

Cons:

* "duplicating" functionality of the existing static constructors -> since the functions would be calling the static constructors anyway, the code is not really duplicated

Yeah. I also made it clear since I started to write this library: avoid repeated code and let the user deal with userland implementations.
This is the reason why I do not provide an abstract Collection class, because the use can implement it himself (see #118).

* global functions might clash with other user-defined functions -> since we use `function_exists`, in cases where `collectIterable` already exists it just wouldn't be defined. Those users would have to choose if they want to change their own function or just use the static constructors

True. I could have use functions for creating all the Collection operations, but I choose not to and created Classes. So for me creating functions for constructors doesn't make sense. This is the reason why we have multiple static constructors in Collection.

* using global functions in general -> these functions are [namespaced](https://github.com/loophp/collection/pull/158/files#diff-41d990dda6814e072d7e35aef8822db3a05cb819602dca63c1ff8d81902060cdR10) so an import would always be needed

Exactly, so, 1 or 2 use statements, I don't really see the point of doing that.

We could argue that the pros are not that significant and I partly agree, but would be nice to hear your reasoning.

I added some feedback below each argument.

I usually like when people contributing inject their ideas, but honestly, for this one I don't follow you 😞.
I think Collection should stay simple and clean. I'm already upset against me because of the Utils/CallbacksArrayReducer.php which shouldn't be there, so, adding helpers... no.

Maybe we should think about all these things in an extra-package, something like loophp/collection-utils where we could provide those things and have more flexibility, user-friendlyness and even more (an abstract Collection class?) ?

@AlexandruGG
Copy link
Collaborator Author

Hello there,

Pros:

* no need for two separate `CollectionInterface` and `Collection` imports (more details in the [discussion](https://github.com/loophp/collection/discussions/135#discussioncomment-1016598))

But why would this be a pro? I don't see any issue of having 1 or 2 use statements in the header of the file. I never find it problematic.

* less characters to write. Might not seem like a big one, but if you you're writing many `Collection::fromIterable` vs `collectIterable` it can make a difference

Yeah... but we have autocompletion and I never write it completely, but rely on my IDE to autocomplete it. So here, I don't see the difference between the 2 options.

Cons:

* "duplicating" functionality of the existing static constructors -> since the functions would be calling the static constructors anyway, the code is not really duplicated

Yeah. I also made it clear since I started to write this library: avoid repeated code and let the user deal with userland implementations.
This is the reason why I do not provide an abstract Collection class, because the use can implement it himself (see #118).

* global functions might clash with other user-defined functions -> since we use `function_exists`, in cases where `collectIterable` already exists it just wouldn't be defined. Those users would have to choose if they want to change their own function or just use the static constructors

True. I could have use functions for creating all the Collection operations, but I choose not to and created Classes. So for me creating functions for constructors doesn't make sense. This is the reason why we have multiple static constructors in Collection.

* using global functions in general -> these functions are [namespaced](https://github.com/loophp/collection/pull/158/files#diff-41d990dda6814e072d7e35aef8822db3a05cb819602dca63c1ff8d81902060cdR10) so an import would always be needed

Exactly, so, 1 or 2 use statements, I don't really see the point of doing that.

We could argue that the pros are not that significant and I partly agree, but would be nice to hear your reasoning.

I added some feedback below each argument.

I usually like when people contributing inject their ideas, but honestly, for this one I don't follow you 😞.
I think Collection should stay simple and clean. I'm already upset against me because of the Utils/CallbacksArrayReducer.php which shouldn't be there, so, adding helpers... no.

Maybe we should think about all these things in an extra-package, something like loophp/collection-utils where we could provide those things and have more flexibility, user-friendlyness and even more (an abstract Collection class?) ?

Fair enough, I think the pros are not that great for this one. Thanks for taking the time to go through this!

@AlexandruGG AlexandruGG deleted the feature/collection-helpers branch July 25, 2021 09:38
@drupol
Copy link
Collaborator

drupol commented Jul 25, 2021

Thanks for the constructive discussion Alex!

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