Skip to content

Conversation

@mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Oct 12, 2020

Initial work in progress support for attachments.

Example integration that uploads stdout/stderr as attachments is here: https://gist.github.com/mitsuhiko/16815c06341370c90030b520984e6ee2

if is_transaction:
envelope.add_transaction(event_opt)
else:
envelope.add_event(event_opt)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhcarvalho I'm starting to think this entire forcing events and transactions to go different paths on the envelope to be wrong-ish. We now need to check if it's a transaction or not to call a different method on the envelope but in both cases they do the same internally. The only difference is that add_event and add_transaction pass different event types.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

I have no real knowledge about Python, so my comments are mostly about the API and not language details. I didn't expect adding attachments to be so simple 😁

@mitsuhiko
Copy link
Contributor Author

Lots of comments, no approvals. Who can sign off on this?

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Had an idea to simplify the API, we may be able to reduce the number of input arguments and usage ambiguity.

cc @philipphofmann @bruno-garcia if that idea would translate well for your platforms of interest.

@mitsuhiko
Copy link
Contributor Author

@philipphofmann i think this is ready to merge.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

The only open question for me is why we don't have any overloads for add_attachment and the init of the attachment or maybe I'm missing something.

Either you want to pass bytes for an attachment or a file to read. So two methods would make sense for me. For the first method, you are forced to pass bytes and a filename for the second you only need to pass a path. This forces you to use the API right at compile time and you don't find out at runtime if you passed in the wrong arguments. We could ditch raising TypeError in the init of the Attachment. What do you think about that @mitsuhiko?

def add_attachment( self, bytes=None, # type: bytes filename=None, # type: str content_type=None, # type: Optional[str] add_to_transactions=False, # type: bool ) def add_attachment( self, path=None, # type: str filename=None, # type: Optional[str] content_type=None, # type: Optional[str] add_to_transactions=False, # type: bool ) 

Other comments are just nitpicking.

@untitaker
Copy link
Member

@philipphofmann there's no such thing as an overload in Python. You can declare an overload in mypy, but that's almost outside of the language.

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@mitsuhiko
Copy link
Contributor Author

Basically what @untitaker said. This is how people do Python APIs :)

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

@philipphofmann
Copy link
Member

@philipphofmann there's no such thing as an overload in Python. You can declare an overload in mypy, but that's almost outside of the language.

Thanks, didn't know that. I never wrote a single line of Python yet 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

7 participants