-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
b06b2b4
to
5871b09
Compare
e6feb12
to
a89ca9f
Compare
b3d4a64
to
04717f6
Compare
04717f6
to
8179f16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo requiring unreleased changes to synapse.
setup.cfg
Outdated
@@ -17,7 +17,7 @@ python_requires = >= 3.7 | |||
[options.extras_require] | |||
dev = | |||
# for tests | |||
matrix-synapse | |||
matrix-synapse @ git+https://github.com/matrix-org/synapse.git@rei/modapi_acc_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be specifying a minimum bound of Synapse somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll add that in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're specifying the version bound as an optional dev-only extra rather than as a hard requirement on the package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this version bound is purely so linters pass (since I assume the module relies on changes that weren't already merged into develop or released in a stable version of Synapse at the time).
Fixes #5.
Requires the module API added in matrix-org/synapse#12391 (likely to land in v1.57.0).
This work is sponsored. It should be possible to review commit-by-commit.