Skip to content

Add Scope::removeTag() method - #1126

Merged
ste93cry merged 1 commit into
getsentry:developfrom
contentking:scope-remove-tag
Nov 6, 2020
Merged

ste93cry merged 1 commit into
getsentry:developfrom
contentking:scope-remove-tag

Conversation

@vojtech-dobes

Copy link
Copy Markdown
Contributor

This was cheap to put together, so feel free to cheaply kill this. But I can't find other way how to remove tag other than clear the whole Scope, which is something I don't believe is desired (I don't want to lose default tags + I don't want to lose tags set by myself in higher level of the code).

@ste93cry ste93cry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apart from fixing the CS and adding an entry in the CHANGELOG, changes are fine for me, however if I don't recall wrong there has been some discussion in the past about adding such removal methods. I'm sure @HazAT can give us some advice on whether we want or not such feature, but I am personally not contrary though

Comment thread tests/State/ScopeTest.php Outdated
@ste93cry

ste93cry commented Nov 4, 2020

Copy link
Copy Markdown
Contributor

Can you please target this PR against the develop branch and can you please reference the PR in the CHANGELOG?

@vojtech-dobes
vojtech-dobes changed the base branch from master to develop November 4, 2020 21:37
@vojtech-dobes

Copy link
Copy Markdown
Contributor Author

@ste93cry Feedback incorporated (changelog, removing one of two, base on develop).

@HazAT HazAT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thx :)

@ste93cry ste93cry added this to the 3.1 milestone Nov 5, 2020
@ste93cry

ste93cry commented Nov 6, 2020

Copy link
Copy Markdown
Contributor

@vojtech-dobes can you 🙏 rebase the PR once more? Build should now be green

@vojtech-dobes

Copy link
Copy Markdown
Contributor Author

@ste93cry done :)

@ste93cry ste93cry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for your hard work!

@ste93cry
ste93cry merged commit 227c716 into getsentry:develop Nov 6, 2020
@vojtech-dobes
vojtech-dobes deleted the scope-remove-tag branch November 6, 2020 20:04
@vojtech-dobes

Copy link
Copy Markdown
Contributor Author

You're welcome!

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.

3 participants