-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add namespaces #49
base: master
Are you sure you want to change the base?
Add namespaces #49
Conversation
@Nacoma Were you able to install this using Composer? I tried installing into a test Laravel project of mine, and I still get:
It seems there is something wrong with trying to import it, which is something that we want to explore fixing. If you happen to be familiar with that issue, it would be great to address that. |
You most likely have a service provider registered in your test project that has not been updated to reflect the changed namespace. Example:
should be:
Failing to update the application will result in the error you're seeing above and (depending on your Laravel version) below.
|
Thank you for that, I guess my unfamiliarity with PHP is showing there. I was able to get that up and running fine now. We are going to have someone from our team review the PR further just to double check everything is good to release. We'll likely need to up the version number with the addition of adding namespacing. Thanks again for contributing here, we really appreciate it. |
May I recommend to change the class names as well? You probably do not want to change the class names ever again since it breaks backwards compatibility. A class name like I think this is a good moment (and maybe the only one for a long time), to think longer about the class names and make them really neat. Maybe there are some other weird things in the naming that we can fix in this mayor breaking change as well. |
I had similar thoughts. An overall rewrite would help this library a lot. There is a lot of duplicate code between resources and manual bootstrapping to make the API calls. The http implementation is very rigid and offers the end user little flexibility, certainly violating the SOLID principles (see issue #1). I think something similar to the current java implementation, and making use of available HTTP libraries, would make this much more developer friendly. |
Either way I agree though. Some renaming and additional structuring would be nice, and will have to happen on a major version bump. It's now or ~never. |
As long as the public interface of the library has the logical public methods and good naming, the refactoring behind the scenes can come later. @mootrichard What are your thoughts on this? If renaming is fine with you, maybe @Nacoma and I can work on the PR together to name the classes and methods? |
I am actually fully onboard with creating a better naming scheme. I find it odd to call Also, I would agree that the refactoring behind the scenes should come later. So long as there isn't a big gap between the previous interface and a newer interface (like the new interface is just more semantic) then it should be fine. |
What is the status of package, is it being updated? |
@lukeholder It appears that it hasn't been updated in at least a year. We've been using it just fine in production for quite a while now on 7.1, but it does seem to be falling behind with no attempts at bringing it up to modern standards. |
Fixes #45, #46, #47
Added namespaces and updated various places that instantiate classes using strings to use the new namespace.