Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Add method to add breadcrumb with string parameter. #500

Conversation

maciejwalkowiak
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Add method to add breadcrumb with string parameter.

💡 Motivation and Context

See #499 (comment)

💚 How did you test it?

There's a unit test.

📝 Checklist

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

🔮 Next steps

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.

👍

@bruno-garcia
Copy link
Member

Oh, what about this also going to master?

@maciejwalkowiak maciejwalkowiak changed the base branch from feat/sentry-java to master July 29, 2020 22:38
@maciejwalkowiak
Copy link
Contributor Author

maciejwalkowiak commented Jul 29, 2020

I'm good if it goes to master. I took today's discussion of not breaking Android build seriously 🙂

Switching base is unfortunately not as straightforward. If @marandaneto has no objections I'll close this one and create another PR against master tomorrow morning.

@maciejwalkowiak maciejwalkowiak changed the base branch from master to feat/sentry-java July 29, 2020 22:40
@bruno-garcia
Copy link
Member

Makes sense to be on the safe side.

This is just an overload so not a breaking change though
For sure Android users can take advantage also, so would be nice.

I believe the main concern are things that would need to be tested on different Android version (yup) to make sure things continue to work. Because, Android.

Things like this overload IMO are safe and will make it simpler later for us to merge things from the java branch and master since a lot of code not ok to merge into master will follow.

@marandaneto thoughts?

@marandaneto
Copy link
Contributor

Makes sense to be on the safe side.

This is just an overload so not a breaking change though
For sure Android users can take advantage also, so would be nice.

I believe the main concern are things that would need to be tested on different Android version (yup) to make sure things continue to work. Because, Android.

Things like this overload IMO are safe and will make it simpler later for us to merge things from the java branch and master since a lot of code not ok to merge into master will follow.

@marandaneto thoughts?

yep, agreed, it's just an overload, things like this are LGTM

@maciejwalkowiak
Copy link
Contributor Author

Closed in favour of #501.

@maciejwalkowiak maciejwalkowiak deleted the add-breadcrumb-string branch July 30, 2020 08:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants