Skip to content
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

Closed
wants to merge 3 commits into from
Closed

WIP - #1048: Better namespace #1077

wants to merge 3 commits into from

Conversation

debo
Copy link

@debo debo commented Apr 15, 2020

? !
Type improvement
BC Break yes
Fixed issues #1048

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.

@noplanman noplanman changed the base branch from master to develop April 18, 2020 00:20
@noplanman
Copy link
Member

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.
Also it would make sense to move the packagist namespace at the same time, to only have one big switch.

@akalongman @jacklul @php-telegram-bot/developers
What's your views on merging this change?

@akalongman
Copy link
Member

I think its ok, but it should be included in a major 1.x.x release.

@akalongman akalongman self-requested a review April 18, 2020 00:27
Copy link
Member

@akalongman akalongman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok

@debo
Copy link
Author

debo commented Apr 18, 2020

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.

@debo
Copy link
Author

debo commented May 16, 2020

I started updating the rest of the code to reflect this change.

php-telegram-bot/ansible-role-php-telegram-bot#1
php-telegram-bot/example-bot#38
php-telegram-bot/laravel#22
php-telegram-bot/support-bot#37
php-telegram-bot/telegram-bot-manager#56

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.

@noplanman
Copy link
Member

Thanks @debo

Basically, we "just" need to get the main changes in that are required to release v1.

To name a few:
#232 #817 #826 #831

@debo
Copy link
Author

debo commented May 17, 2020

No problem, I'll have a look at those in the next days maybe and try to help you out with those too.

@noplanman
Copy link
Member

@debo Looks like the tests are failing because the namespaces of the test files need to be updated too.

@debo
Copy link
Author

debo commented May 24, 2020

@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 EntityTest.php file I mean.

@noplanman
Copy link
Member

@debo Looks like your branch wasn't rebased / updated correctly, so I've merged develop in and fixed the problematic namespace declaration 👍

@debo
Copy link
Author

debo commented May 24, 2020

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.

@TiiFuchs
Copy link
Member

We're doing a partial rewrite for v1 and consider this during the development.

@TiiFuchs TiiFuchs closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants