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.

Sign up for GitHub

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

Jump to bottom

Add tags to container #144

Merged
merged 14 commits into from
Apr 15, 2021
Merged

Add tags to container #144

merged 14 commits into from
Apr 15, 2021

Conversation

yiiliveext
Copy link
Contributor

@yiiliveext yiiliveext commented Sep 1, 2020 •
edited by samdark
Loading

Q A
Is bugfix? ❌
New feature? ✔️
Breaks BC? ✔️
Fixed issues #125

@yiiliveext yiiliveext requested a review from a team September 1, 2020 12:39
thenotsoft
thenotsoft reviewed Sep 1, 2020
View reviewed changes
src/Container.php Outdated Show resolved Hide resolved
thenotsoft
thenotsoft reviewed Sep 1, 2020
View reviewed changes
src/Container.php Outdated Show resolved Hide resolved
@thenotsoft thenotsoft added the status:code review The pull request needs review. label Sep 1, 2020
@samdark samdark requested a review from a team September 1, 2020 22:56
samdark
samdark approved these changes Sep 2, 2020
View reviewed changes
Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

The idea itself is alright 👍 Need at least some docs to merge it.

src/Container.php Outdated Show resolved Hide resolved
src/Container.php Outdated Show resolved Hide resolved
src/Container.php Outdated Show resolved Hide resolved
@samdark
Copy link
Member

samdark commented Sep 2, 2020

Need docs to get an idea about how it is intended to be used.

@yiiliveext yiiliveext mentioned this pull request Sep 2, 2020
Closed
xepozz
xepozz reviewed Sep 2, 2020
View reviewed changes
Copy link
Member

@xepozz xepozz left a comment

Choose a reason for hiding this comment

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

What's the result will be for calling $container->has('tag@engine')?

@yiiliveext
Copy link
Contributor Author

What's the result will be for calling $container->has('tag@engine')?

Good question.

@xepozz xepozz mentioned this pull request Sep 6, 2020
Closed
@xepozz
Copy link
Member

xepozz commented Sep 6, 2020 •
edited
Loading

Will a composite container collect all tags definitions from subcontainers?

@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Sep 6, 2020
@yiiliveext
Copy link
Contributor Author

Will a composite container collect all tags definitions from subcontainers?

Done

@samdark samdark added status:code review The pull request needs review. type:docs Documentation and removed status:under development Someone is working on a pull request. labels Sep 21, 2020
@samdark
Copy link
Member

samdark commented Sep 21, 2020 •
edited
Loading

Ready for merge. Docs are needed.

@samdark samdark added status:ready for merge The pull request is OK to be merged. and removed status:code review The pull request needs review. labels Sep 21, 2020
@hiqsol
Copy link
Member

hiqsol commented Sep 23, 2020

Please see alternative proposal #152
It provides almostly the same but is much simpler.

@samdark
Copy link
Member

samdark commented Sep 29, 2020

@yiiliveext would you please resolve conflicts?

@yiiliveext
Copy link
Contributor Author

@yiiliveext would you please resolve conflicts?

Done

@samdark samdark requested review from samdark and hiqsol April 15, 2021 15:36
@samdark samdark removed the type:docs Documentation label Apr 15, 2021
@samdark samdark mentioned this pull request Apr 15, 2021
Closed
@samdark samdark merged commit 13d0452 into yiisoft:master Apr 15, 2021
@yiiliveext yiiliveext deleted the tags branch August 3, 2021 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BoShurik BoShurik BoShurik left review comments

@xepozz xepozz xepozz left review comments

@thenotsoft thenotsoft thenotsoft left review comments

@samdark samdark samdark approved these changes

@hiqsol hiqsol Awaiting requested review from hiqsol

Assignees
No one assigned
Labels
status:ready for merge The pull request is OK to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yiiliveext @samdark @xepozz @hiqsol @BoShurik @thenotsoft