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

Test util create_room_as is_public option is misleading #10951

Closed
DMRobertson opened this issue Sep 30, 2021 · 0 comments · Fixed by #10963
Closed

Test util create_room_as is_public option is misleading #10951

DMRobertson opened this issue Sep 30, 2021 · 0 comments · Fixed by #10963
Labels
good first issue Good for newcomers P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@DMRobertson
Copy link
Contributor

Defaults to True.

is_public: bool = True,

Docstring says:

is_public: If True, the `visibility` parameter will be set to the
default (public). Otherwise, the `visibility` parameter will be set
to "private".

But the only use is:

if not is_public:
content["visibility"] = "private"

Moreover, the CS spec says of visibility that

Rooms default to private visibility if this key is not included.

Suggest:

  • Change is_public to Optional[bool], default None
  • if is_public is False, set visibility to private
  • elif is_public is True, set visibility to public
  • else do nothing.

Could alternatively leave it as a bool and fix it to actually set visibility to public when True. That would probably require fixing a bunch of tests to explicitly set is_public: False.

Spotted by @AndrewFerr in #10930.

@DMRobertson DMRobertson added good first issue Good for newcomers S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches labels Sep 30, 2021
@DMRobertson DMRobertson linked a pull request Oct 1, 2021 that will close this issue
4 tasks
DMRobertson pushed a commit that referenced this issue Oct 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant