-
-
Notifications
You must be signed in to change notification settings - Fork 960
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
WIP - #1048: Better namespace #1077
Conversation
Hi @debo Thanks for the PR. As mentioned in the issue, this is a huge BC break and all code examples in the issues that have the namespace in them will become invalid. @akalongman @jacklul @php-telegram-bot/developers |
I think its ok, but it should be included in a major 1.x.x release. |
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.
Looks ok
Yes, I appreciate is a breaking one and it requires a major, I just wanted to help in the meantime. I’m going to update the examples and the docs where required because I didn’t think about those. Marking this as WIP in the meantime then. |
I started updating the rest of the code to reflect this change. php-telegram-bot/ansible-role-php-telegram-bot#1 Builds are failing because I can't really update the lock files because that will require you people to update packagist but I'm sure we can coordinate on that. Please let me know how to proceed now, thanks and I hope it helps. |
No problem, I'll have a look at those in the next days maybe and try to help you out with those too. |
@debo Looks like the tests are failing because the namespaces of the test files need to be updated too. |
@noplanman which is already the case if you look at the PR... also, unless I'm overlooking something this Class 'Longman\TelegramBot\Tests\Unit\TestCase' not found in /home/travis/build/php-telegram-bot/core/tests/unit/Entities/EntityTest.php:23 Doesn't even exists, the |
@debo Looks like your branch wasn't rebased / updated correctly, so I've merged develop in and fixed the problematic namespace declaration 👍 |
That's weird... it was freshly forked and rebase from upstream is one of the first thing I do as part of my workflow. Glad we sorted this out. I'm going to have a look at the build for the bot manager which is failing as well. |
We're doing a partial rewrite for v1 and consider this during the development. |
Summary
As mentioned in the opened issue/feature request this PR is aimed to align the project namespace to the actual project name for consistency.
I couldn't run the DB integration tests because I don't have a proper local setup but I noticed travisci will take are of that.
I hope it helps.