Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Join rules and guest access #174

Merged
merged 3 commits into from
Apr 23, 2018
Merged

Join rules and guest access #174

merged 3 commits into from
Apr 23, 2018

Conversation

Zil0
Copy link
Contributor

@Zil0 Zil0 commented Feb 25, 2018

Hello, aspiring GSoC student here :)
I tried to fix #158, and doing guest access too as the two events are related and very similar.
However I have some questions :

  • Should I add getters, tests, listeners ?
  • Should I add join_rules as a room attribute ?
  • The issue mentions client.py, but it seemed more logical to me to add the methods to room.py, did I get that right ?

Thanks !

@non-Jedi non-Jedi self-requested a review February 26, 2018 03:13
@non-Jedi
Copy link
Collaborator

Hi @zilo. Sorry about the delay; really low on time here. To answer questions:

  • not sure what you mean be getters. Test would be good. Listeners would be
    good but relatively low priority.
  • I think so, ya.
  • Yep. Good job.

Lmk if any other questions. Pinging @ara4n since this is a potential in case I
go missing for two weeks again.

@non-Jedi non-Jedi removed their request for review March 11, 2018 03:34
Fix #158.
Also add a new room attribute and tests.
Also add a new room attribute and tests.
@Zil0
Copy link
Contributor Author

Zil0 commented Mar 14, 2018

Thanks a lot for your answers! Should be better now, I added tests and room attributes.
Getters were based on a misconception I had about state events, cleared by reading more doc :)

@non-Jedi
Copy link
Collaborator

@Zil0 is this ready to be reviewed? The title still says "WIP", so I wasn't sure.

@Zil0 Zil0 changed the title WIP: Join rules and guest access Join rules and guest access Mar 30, 2018
@Zil0
Copy link
Contributor Author

Zil0 commented Mar 30, 2018

@non-Jedi it is, thanks for the heads-up

@Zil0
Copy link
Contributor Author

Zil0 commented Apr 17, 2018

Forgot to add:
Signed-off-by: Valentin Deniaud valentin.deniaud@inpt.fr

@@ -624,6 +625,24 @@ def modify_required_power_levels(self, events=None, **kwargs):
except MatrixRequestError:
return False

def set_invite_only(self, invite_only):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really think we should make this take a (fake) enum rather than a boolean
(change method name also obviously). That way if in the future knock or
private are used, we can accommodate that without changing the API for this
class. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to have something intuitive. As I understood it, the goal of those higher-level methods is to ease the use of the CS API, hence it didn't make sense to me to have basically the same method in room.py and api.py.
Also, I'm thinking that if knock or private are used, they will necessitate changes to this method anyway, so it may be good to wait to include them. And depending on what behavior they introduce, maybe it will make sense to have two different setters for the same underlying api call!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You convinced me. Thanks again!

@non-Jedi
Copy link
Collaborator

Modulo that single comment, This LGTM. Thanks!

@non-Jedi non-Jedi merged commit 36705b0 into matrix-org:master Apr 23, 2018
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.

Send m.room.join_rules event
2 participants