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

ref: Doesnt allow for popping last layer and unify getter methods #2955

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

kamilogorek
Copy link
Contributor

Preventing the popping of the last layer makes it in line with sentry-python behavior. It didn't make much sense to allow that in the first place. Because of that change, during construction, it'll update the initially empty layer instead of pushing a new one.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2020

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 18.02 KB (+0.03% 🔺)
@sentry/browser - Webpack 18.84 KB (-0.01% 🔽)
@sentry/react - Webpack 18.84 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 23.9 KB (-0.01% 🔽)

@kamilogorek kamilogorek requested review from rhcarvalho and HazAT and removed request for rhcarvalho October 7, 2020 10:54
@kamilogorek kamilogorek merged commit dd8e392 into master Oct 7, 2020
@kamilogorek kamilogorek deleted the hub-refactor branch October 7, 2020 11:37
if (finalBreadcrumb === null) {
return;
}
if (finalBreadcrumb === null) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of if without {}, a bit harder to read when scanning quickly through code, easier to introduce bugs. (isn't there a linter rule for this?!)

On top of that this change is introducing inconsistency in style. For example, later in this file:
image

@@ -105,7 +103,8 @@ export class Hub implements HubInterface {
* @inheritDoc
*/
public popScope(): boolean {
return this.getStack().pop() !== undefined;
if (this.getStack().length <= 1) return false;
return !!this.getStack().pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be accessing this._stack directly, same in pushScope.

Why is there even a public getStack()?! Is there a use case to deal with stack layers from code external to this class?!

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.

None yet

3 participants