search

telkins / laravel-dag-manager

A SQL-based Directed Acyclic Graph (DAG) solution for Laravel.
MIT License
32 stars 9 forks source link

Overall improvements #13

Closed akalongman closed 3 years ago

akalongman commented 3 years ago

This PR is a first step for making the package better. Notable changes:

@telkins we use this package for our project and this changes is done in that scope. Maybe there are many changes and if it's goes against your contributing policy, feel free to just close the PR :relaxed:

Related with #12

telkins commented 3 years ago

Hey....just a quick note to let you know that your PR has been seen. I will take a close(r) look at it when time permits, which should be sometime over the next several days. I know there are some (small) things that I might have questions/concerns about, but I don't want to get ahead of myself before I get a chance to really sit down and look it over.

The main point of this message was to let you know that your PR has been seen and it's high on my list to take care of....but I just need to get a good window to do so. 🤓

telkins commented 3 years ago

@akalongman I'm sorry I haven't had time to look at this these last few days, but I haven't forgotten about it. I'd like to start looking at it more closely now, but I can't guarantee how much I can do or how quickly.

Whatever the case, let me say that I appreciate your contribution and the work you have put into it. It makes me happy that others are using this package and some are willing/able to look for ways to make it better. 🤓

So...right off the bat, I have some questions. Please answer these as best you can when you have the opportunity:

  1. Why do you think it's a good idea to remove support of Laravel lower than 8.0...? I worry about users who haven't upgraded to L8 yet.
  2. Why do you think it's a good idea to remove support of PHP lower than 7.4...? Again, I I worry about users who haven't upgraded to PHP 7.4 yet.
  3. Why make code more strict...? When this was first available, I thought it would be a good thing, but, if memory serves, I eventually came to the conclusion that it didn't help quite as much as I thought it would.

I think that's it for now. I know I have some other questions that will require a bit more discussion. I'll ask those in a different comment or in the code.

Also, please don't think of these questions as arguments. They're not meant to be presented as such. 🤓

telkins commented 3 years ago

@akalongman With regards to making the service provider deferrable, I had some more questions.

I'm looking at the documentation and see the following:

If your provider is only registering bindings in the service container, you may choose to defer its registration until one of the registered bindings is actually needed.

I guess my concern was related to the fact that the service provider has a boot method. Does this mean that doesn't "only register bindings"...?

I looked for examples of where DeferrableProvider was used in Laravel and it seems like it's never used for any service provider other than those that only register bindings. None of them have boot methods, for example. Likewise, I searched in all of the other packages in one of my bigger projects and only found one instance of a deferred provider....Spatie's laravel-query-builder...sort of. 😝 It has a TODO comment suggesting its use once support for L5.7 is dropped.

So...I think it's a good idea, but want to make sure that having the boot method doesn't present a problem. If you could help shine a light on this whole subject, I'd appreciate it. 🤓

akalongman commented 3 years ago

@telkins sorry for the late answer :) Happy New Year! I am glad this PR was merged, thank you. :+1:

telkins commented 3 years ago

@telkins sorry for the late answer :) Happy New Year! I am glad this PR was merged, thank you. 👍

Happy New Year to you, too...! 🥳

Thanks again for the effort you put into it. Hope you saw my tweet, too: https://twitter.com/traviselkins/status/1345396258496778241?s=20 🤓

Take care...

akalongman commented 3 years ago

Hope you saw my tweet

Did not see it before, thanks :smile:

telkins commented 3 years ago

Hope you saw my tweet

Did not see it before, thanks 😄

I like to give credit where credit's due. 🤓