Skip to content

Conversation

@marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Oct 21, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

feat: before breadcrumb and scope ref

💡 Motivation and Context

One should be able to filter breadcrumbs on before breadcrumb callback and also add event processors to the scope

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

Contexts is still missing

@marandaneto marandaneto changed the base branch from main to feature/unified-api October 21, 2020 09:28
@marandaneto marandaneto changed the title feat/before breadcrumb feat: before breadcrumb and scope ref Oct 21, 2020
@marandaneto marandaneto requested a review from rxlabz October 21, 2020 09:32
clone.addBreadcrumb(breadcrumb);
}

for (final eventProcessor in _eventProcessors) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we're in the class, what about a private constructor or factory method that takes all fields as named args?
Then we could just clone all lists etc like Scope({eventProcessors = [...this.eventprocessors]) and avoid tall these for

Copy link
Contributor Author

@marandaneto marandaneto Oct 21, 2020

Choose a reason for hiding this comment

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

mm yeah but rethinking this right now, maybe it'd make sense to just do a copyWith and follow the convention? tbh I just added the missing field in the existent method, I will check whats the state of the art for cloning objects on Dart.
Factory is public afaik, but a private ctor would work as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://pub.dev/documentation/copyable/latest/#examples
using the copyable interface seems to be the standard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add this as a task on our project, so I can merge this now

Copy link
Contributor

Choose a reason for hiding this comment

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

copyWith is probably the "most standard way" ( used a lot in Flutter source code, and in https://pub.dev/packages/freezed, a popular immutable package)

@marandaneto marandaneto merged commit 3fa0b79 into feature/unified-api Oct 22, 2020
@marandaneto marandaneto deleted the feat/before-breadcrumb branch October 22, 2020 08:13
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