Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Convert EventContext to attrs#6218

Merged
hawkowl merged 7 commits intodevelopfrom
rav/event_auth/3
Oct 28, 2019
Merged

Convert EventContext to attrs#6218
hawkowl merged 7 commits intodevelopfrom
rav/event_auth/3

Conversation

@richvdh
Copy link
Copy Markdown
Member

@richvdh richvdh commented Oct 18, 2019

The main driver for this was to get a sensible __repr__, but the improved type-safety doesn't hurt.

@richvdh richvdh requested a review from a team October 18, 2019 17:49
self.prev_state_events = []
self.rejected = False
self.app_service = None
state_group = attr.ib(default=None)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just a note: you can annotate the types in this with type= -- for the optional ones you want to use typing.Optional[int] or whatever. mypy can read that info, and also some editor IDEs can as well, so it can be worth it :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

mmm, good to know. I'll consider it next time ;)

Copy link
Copy Markdown
Contributor

@hawkowl hawkowl left a comment

Choose a reason for hiding this comment

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

On the old deps builders:

builtins.TypeError: attrib() got an unexpected keyword argument 'factory'

This is an 18.1.0 feature in attrs and we depend on a min of 17.4. We should probably upgrade to 18.1, or fix this here to use default=attrs.Factory() -- your choice (I prefer the former :) ).

@richvdh richvdh requested review from a team and hawkowl October 24, 2019 10:50
@hawkowl hawkowl merged commit 9aee289 into develop Oct 28, 2019
@richvdh richvdh deleted the rav/event_auth/3 branch December 10, 2019 17:44
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '9aee28927':
  Convert EventContext to attrs (#6218)
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