Skip to content

Conversation

@ueman
Copy link
Collaborator

@ueman ueman commented Mar 13, 2021

📜 Description

This is a breaking change.

This PR renames a couple classes:

  • App -> SentryApp
  • Browser -> SentryBrowser
  • Device -> SentryDevice
  • Message -> SentryMessage
  • Request -> SentryRequest
  • User -> SentryUser
  • Gpu -> SentryGpu
  • OperatingSystem -> SentryOperatingSystem
  • Integration -> SentryIntegration

I wonder if we should rename the following classes too:

  • Breadcrumb
  • Contexts

The following classes are probably fine:

  • DebugImage
  • DebugMeta
  • Dsn
  • Mechanism
  • SdkInfo
  • SdkVersion

💡 Motivation and Context

See #250

💚 How did you test it?

No functional changes, tests are still green

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

Update documentation:

@ueman ueman changed the title Fix: Rename most classes Fix: Rename some protocol classes Mar 13, 2021
@ueman ueman marked this pull request as ready for review March 15, 2021 06:30
CHANGELOG.md Outdated
* Fix: Multiple FlutterError.onError calls in FlutterErrorIntegration (#345)
* Fix: Pass hint to EventProcessors (#356)
* Fix: EventProcessors were not dropping events when returning null (#353)
* Fix: Prefix App, Browser, Device, Message, Request and User with Sentry (#250)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Fix: Prefix App, Browser, Device, Message, Request and User with Sentry (#250)
Breaking Changes:
* Fix: Prefix App, Browser, Device, Message, Request and User with Sentry (#250)
// let's describe here the renamed Protocols and that now they have a 'Sentry' prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

we'll probably need to add a Migration page to https://docs.sentry.io/platforms/dart/ but let's track this in another issue :)

@marandaneto
Copy link
Contributor

marandaneto commented Mar 15, 2021

I guess Breadcrumb and Contexts are fine.
OperatingSystem, Integration and Gpu I'd have renamed though, now with more supported platforms, that might become a problem too.
@bruno-garcia @philipphofmann ?

@marandaneto marandaneto requested a review from a team March 15, 2021 08:36
ueman and others added 3 commits March 15, 2021 11:17
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

LGTM but I wonder if we're going into 5.0.0 now?

@bruno-garcia
Copy link
Member

OperatingSystem, Integration and Gpu I'd have renamed though, now with more supported platforms, that might become a problem too.
@bruno-garcia @philipphofmann ?

Agree renaming these 3 could make sense.

@codecov-io
Copy link

codecov-io commented Mar 15, 2021

Codecov Report

❌ Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.80%. Comparing base (00417e1) to head (312c212).
⚠️ Report is 1866 commits behind head on main.

Files with missing lines Patch % Lines
dart/lib/src/protocol/contexts.dart 81.81% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   89.79%   89.80%   +0.01%     
==========================================
  Files          51       51              
  Lines        1636     1638       +2     
==========================================
+ Hits         1469     1471       +2     
  Misses        167      167              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marandaneto
Copy link
Contributor

LGTM but I wonder if we're going into 5.0.0 now?

let's do one more pre-release with the fixes we've done and if all good, we do a GA next.

@marandaneto
Copy link
Contributor

OperatingSystem, Integration and Gpu I'd have renamed though, now with more supported platforms, that might become a problem too.
@bruno-garcia @philipphofmann ?

Agree renaming these 3 could make sense.

@ueman do you prefer to rename these 3 left on this PR or a new one?

@ueman
Copy link
Collaborator Author

ueman commented Mar 15, 2021

OperatingSystem, Integration and Gpu I'd have renamed though, now with more supported platforms, that might become a problem too.
@bruno-garcia @philipphofmann ?

Agree renaming these 3 could make sense.

@ueman do you prefer to rename these 3 left on this PR or a new one?

I've renamed them in this PR.

@philipphofmann
Copy link
Member

philipphofmann commented Mar 15, 2021

For consistency, we could prefix all of them with Sentry. As a user, I can imagine, that you don't fully understand why some of them have the prefix and some of them don't.

@marandaneto marandaneto merged commit 100fd51 into main Mar 15, 2021
@marandaneto marandaneto deleted the fix/rename branch March 15, 2021 15:04
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.

6 participants