search

nikic / FastRoute

Fast request router for PHP
Other
5.1k stars 445 forks source link

Dispatcher result refactored, RouteCollector to RouteCollection renamed #223

Closed burzum closed 3 years ago

burzum commented 3 years ago
burzum commented 3 years ago

@lcobucci so yet another month has passed, any chance I'm getting answers to my comments please? By this speed we'll have a new version sometimes around 2050. 😭

lcobucci commented 3 years ago

@lcobucci so yet another month has passed, any chance I'm getting answers to my comments please? By this speed we'll have a new version sometimes around 2050. 😭

First of all, this is OSS. We're not on the clock to ship anything. I'm doing my best to help this go through. If that isn't enough for you, sorry, but that's what I have to offer for now.

burzum commented 3 years ago

@lcobucci just because something is OSS doesn't mean that somebody who is an active maintainer jumps in every few month. A maintainer has responsibility as well. Using OSS as an excuse is weird. It is very demotivating for the people who contribute and won't receive a response within a reasonable timeframe. Especially if you invest time, that is not covered by a company, and then get more or less ignored for weeks. I would prefer a statement like "Hey, I don't care!" than having to wait and trying to make the changes in a way everyone gets something out of it. I could also just make them for myself and don't care about contributing. 🤷 Sharing code and working together is the actual spirit of OSS, at least for me. We an discuss this with a 🍺 at some conference if you like (after this damn pandemic). 😃

This PR is green, it should be almost fully BC. This could be released as 2.0 and then a 3.0 could be started to remove the deprecated things and make more breaking changes. This way it will provide a smooth migration path.

lcobucci commented 3 years ago

@burzum I understand your complaints, expectations, and demotivation. The part that it's being frustrating and hard in this process is to help you understand my side as well.

What made me bring the OSS argument is the fact that my time is just as important as yours. And I feel disrespected when, despite having asked you to split things up in different PRs so we can have more focused discussions, you kept increasing the scope.

We don't know each other and we have very little context about what happens behind the keyboards on each side of this PR. That also doesn't help much 😅

Our approaches to development seem to be quite different but we also seem to care about similar things.

My general opinion about this PR didn't change. I feel like it has too many things. There are great ideas too. I just disagree with execution and the impact of that on the design of the lib - hence my requests to split and work together to find a good balance.

I'd love to have a synchronous chat with you to make things move forward and (hopefully) be able to share with you a little about my side and show you that I do care.

burzum commented 3 years ago

@lcobucci OK... Any suggestion on how to split this set of changes up without causing me a ton of more work? I don't want to spend hours just to break it up.

lcobucci commented 3 years ago

@burzum I'll have a closer look at it tomorrow and suggest something - I can definitely help with git stuff to make sure things don't take long.

lcobucci commented 3 years ago

My proposal for splitting this is to incrementally extract the changes related to each topic.

We'd start by squashing all the changes in a single commit on a reference branch, then we create a branch for the first topic we'll extract (based on the reference branch). The extraction consists of resetting that commit, picking the necessary changes, logically committing things, and sending a PR.

Once the PR is created, the reference branch is rebased to remove the extracted changes from the reference commit. After merging the PR, the reference branch is synced again just to make sure we're good.

We can share the load on extracting the code - I've done this kind of thing several times, so am quite comfortable with that. We can also try to do that together via any tool (google meet/zoom/${your-tool-of-choice-here}). I'd like for you to have your name on the stuff you're doing, so I'd prefer to have PRs created by you from your fork.

If you agree, we can convert this PR into a draft to make it the reference and start the extraction. I'd start with the .editorconfig and the using an object as the dispatching result.

What do you think?

burzum commented 3 years ago

I won't have much time this week, next week maybe? Thuesday evening? Zoom, Skype, Slack, Google, Discord... whatever you want, after more than 12 month pandemic I have them all. 😆

By the way, meanwhile I've got an ugly but working prototype of reverse routing. It could probably optimized and it needs to handle the case when some non-optional args are missing. It should cover both cases, generating a route with all args and even without the optional args. I haven't yet added a route factory but there is for sure one required if SOLID is wanted. :)

But I've ditched most BC except the BC of the result of the dispatcher in that branch and also replaced the creation of the result object with a factory and renamed the interfaces to suffix them with interface, so they match the standards I'm used to. I dislike when they're discarded, makes it easy to think it's a class, even with an IDE using shiny icons.

https://github.com/burzum/FastRoute/blob/fastroute-ng/src/

Edit: Just added the RouteFactory.

lcobucci commented 3 years ago

Next week sounds good to me :+1: Ping on me Twitter or Discord (lcobucci#1456).

In the meantime, I'll rebase this branch to have it squashed and synced to master :+1:

lcobucci commented 3 years ago

The new CI (GH-based) is now failing due to some new and advanced checks I added. We'll solve them during the extraction process, so let's not worry too much now :+1:

burzum commented 3 years ago

@lcobucci do you want the changes from https://github.com/burzum/FastRoute/blob/fastroute-ng/src/ as well? I've added reverse routing and named routes and finished getting everything into factories. Architecture wise it's done, I might just need to refine some methods and find edge cases. Feature wise the refactor is completed. I've added there typed properties, so it won't work below 7.4. 7.3 is EOL by the way. It contains everything I wanted for now.

burzum commented 3 years ago

I fixed all phpstan errors and some doc blocks in https://github.com/burzum/FastRoute/blob/fastroute-ng/src/ and also increased phpstan to level 8.