Skip to content

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Feb 4, 2021

During todays attempt to implement #265, i found it increasingly difficult to

  • work within the class-based implementation
  • deal with the classes that are passed to the callbacks defined within the options

In my experience, plain objects and (mostly) pure functions lead to simpler code that is easier to work with, less stateful and less error prone. What do you think about this approach?

This PR is just a draft as of now. If you don't like it, that is totally fine, i can also maintain it in my fork. Perhaps, we can find common ground and continue working on this together. Next steps for me would be to add spans and some performance tracing.

In any case thank you for your previous work on this, i found the general structure and options to really make sense.

@DiederikvandenB
Copy link
Owner

I wanted to look at this today but I didn’t find the time for it. I am definitely open for refactoring and I’ll have a closer look after the weekend.

@spawnia spawnia marked this pull request as ready for review February 6, 2021 18:32
@spawnia
Copy link
Collaborator Author

spawnia commented Feb 6, 2021

Not quite ready to merge, but complete enough to be worth a review.

@spawnia spawnia marked this pull request as draft February 8, 2021 13:17
@DiederikvandenB
Copy link
Owner

Thanks for taking the time to write this PR.

I mostly decided to go the class way because Sentry's class based too, but I like your approach as well. Let me know when it's ready to be merged!

@spawnia
Copy link
Collaborator Author

spawnia commented Feb 8, 2021

I am going to be battle-testing this for a few more days with the current feature set. How should I best document the changes? How about an UPGRADE.md for the breakage?

@DiederikvandenB
Copy link
Owner

DiederikvandenB commented Feb 9, 2021

An UPGRADE.md sounds good.

@spawnia
Copy link
Collaborator Author

spawnia commented Feb 11, 2021

Did some more testing with it, everything is working fine. I rewrote the commit messages and added both changelog and upgrade guide. Everything should be ready to merge :)

@spawnia spawnia marked this pull request as ready for review February 11, 2021 11:54
@DiederikvandenB DiederikvandenB merged commit eeb21a5 into DiederikvandenB:master Feb 15, 2021
@DiederikvandenB
Copy link
Owner

Cheers. Merged & released.

@spawnia spawnia deleted the functional branch February 15, 2021 12:09
@spawnia
Copy link
Collaborator Author

spawnia commented Feb 15, 2021

Thank you. Next up, support for spans :)

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.

2 participants