-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
SA Checks: Methods Starting with "a" #97
SA Checks: Methods Starting with "a" #97
Conversation
@drupol I think what I initially intended to do with these static-analysis checks is proving a bit infeasible 😓 . I think I didn't realise the flexibility that the operations actually allow because I mostly looked at the type hints, which are more restrictive at the moment than what the library allows you to do. For example Associateable suggests that you will get back the same In addition, most of the examples in the documentation are with collections of a single type. I think that in order to allow for usage as intended, with a lot of flexibility, things need to be a bit more loosely-typed. That's probably one of the reasons why the Laravel Collections typings are a lot less specific, i.e. more usage of |
Thanks for being honest, I fully agree and I know that we will loose some typing information because of that flexibility. That's the trade off. What do you think we should do in order to clean up the types in the library then? |
I think we need to remove some of the template types and allow things to be less specific and The best way I see this happening is in parallel: for example let's say I'm adding the static analysis checks for the |
@drupol what do you think about this approach? If you're okay with it I can start a new PR so you can see what it would look like, and for this PR I would remove most things apart from the |
Hi Alex, Sorry for the delay in my responses. I'm preparing my presentation at AFUP about this library, this is tomorrow: https://event.afup.org/afupday2021-interview-pol-dellaiera/ Regarding your question, yes I agree obviously. As long as we won't use a typed collection, we will have to use |
$fact = static fn (float $number): float => (float) Collection::range(1, $number + 1) | ||
->foldLeft($multiplication, 1) | ||
->current(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was causing a failure in CI and confusing me massively 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sometimes those tools are extremely frustrating!
No clue why unit tests are failing on macOS in CI but it's unrelated to the changes here 🤷♂️ |
Thanks mate! Now going to reply to your email :) |
This PR initially aimed to add to the SA Checks all collection methods starting with the letter "a".
After the discussions and the realisation that more
mixed
typing usage will be needed, I decided to shorten this to only the static analysis check forall
+ a few other minor changes.