-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Fix optional parameters in url generator #2246
Fix optional parameters in url generator #2246
Conversation
…ent, add unit tests
[ci skip] [skip ci]
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.
I'm ok with the approach and the unit tests seem fine. I do find the actual implementation quite difficult to read.
Could you try to make clear which variables reference an index and which variables reference a count?
I'm not actually sure what our preference is between snake_case and camelCase but I was under the impression we mostly used camelCase?
I'll wait to review until the things clark pointed out are fixed. Like clark I also have difficulty reading the actual implementation so some clarification of variables would be great. |
Oh boy this code is a mess, not sure what I was thinking when I wrote it... I did a bit of cleanup, that was embarrasing... |
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.
That looks much better.
I still find the use of the index as the "number" of parameters a bit confusing. This only works because we expect all $parts
to be a repetition of the same elements with further entries just having more elements after the common elements.
This might not be obvious to code readers unfamiliar with Fastroute.
If all $parts
are always sorted in ascending number of parts, maybe this could even be simplified to break
as soon as one result doesn't have as many matches as the previous entry. Not sure if the optimization is worth it though as most routes will probably only ever have two routes variants at most.
Fixes #1972
Changes proposed in this pull request:
Reviewers should focus on:
Confirmed
composer test
).