Add support for all streams (more general accessor methods) #1419

theViz343 · 2023-07-27T12:40:05Z

What does this PR do, and why?

This PR adds support for unsubscribed streams and never_subscribed streams in ZT. It creates new data structures and general accessor methods to access them. This PR is needed for further work on reducing stream related issues and adding support for stream (un)subscription.
This PR is based heavily on the initial commits of #1408 with some slight changes and the generalization of the accessor methods.

Outstanding aspect(s)

Does the typing for the accessor methods look fine?

External discussion & connections

Discussed in #zulip-terminal in Adding streams throws key error
Fully fixes Adding new stream throws an Keyerror #816
Partially fixes issue #
Builds upon previous unmerged work in PR #
Is a follow-up to work in PR #
Requires merge of PR #
Merge will enable work on Handle (un)subscribe events #1387

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

zulipbot · 2023-07-27T12:40:09Z

Hello @theViz343, it seems like you have referenced #816 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #816..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

neiljp

@theViz343 This has the potential to be simpler than the other PR, though I'm a little concerned over if you may end up with typing issues?

neiljp · 2023-08-01T02:21:09Z

zulipterminal/api_types.py

@@ -244,6 +233,21 @@ class Subscription(TypedDict):
    # in_home_view: bool  # Replaced by is_muted in Zulip 2.1; still present in updates


As per the other PR, note that this is the successor to is_muted, so should move too.

neiljp · 2023-08-01T03:22:30Z

tests/conftest.py

@@ -243,21 +244,26 @@ def general_stream() -> Dict[str, Any]:
        "audible_notifications": False,
        "description": "General Stream",
        "rendered_description": "General Stream",
-        "is_old_stream": True,


Worth noting why these removals are safe in the commit message.

neiljp · 2023-08-01T06:26:05Z

tests/core/test_core.py

        controller.model.stream_dict = {
-            stream_id: {
+            stream_id: general_stream,
+        }


Is this change strictly required/enforced by the new typing of stream_dict?

Yes, the new typing makes it so that each item in the stream_dict has to be of the Subscription Typeddict type.

neiljp · 2023-08-01T06:27:04Z

tests/core/test_core.py

+            205: general_stream,
+        }
+        controller.model.stream_dict[205].update(


This may be a reasonable commit to use a fixed index via a variable, as in the other tests?

neiljp · 2023-08-01T06:29:59Z

tests/conftest.py

@@ -1453,7 +1509,7 @@ def user_id(logged_on_user: Dict[str, Any]) -> int:


 @pytest.fixture
-def stream_dict(streams_fixture: List[Subscription]) -> Dict[int, Subscription]:
+def stream_dict(streams_fixture: List[Dict[str, Any]]) -> Dict[int, Any]:


Accidental revert?

neiljp · 2023-08-01T07:01:29Z

zulipterminal/model.py

+    def stream_property(
+        self, stream_id: int, property: STREAM_KEYS
+    ) -> Optional[Union[int, str, bool, List[int]]]:
+        return self._get_stream_from_id(stream_id)[property]


How well does this approach work in practice? Does this lead to typing errors, since somehow one has to map the result types to the property names?

This commit creates data structures, similar to stream_dict, to support unsubscribed and never-subscribed streams. These streams are available in the fetched initial data which is used to populate the new data structures using the _register_non_subscribed_streams function.

This commit updates the `date_created` and `role` fields present in the Stream and Subscription typeddicts respectively. For `date_created`, since servers with ZFL<30 do not include the field, but ZT does add it with a value of None to ensure consistency, the type is changed to `NotRequired[Optional[int]]`. For `role`, the field has been removed in Zulip 6.0 (ZFL 133), so it has been removed from the Subscriptions typeddict.

With the revamp of the Subscription TypedDict in an earlier commit, we use it to improve the typing of stream_dict in this commit. Tests and fixtures updated to support the new typing.

This commit adds two helper methods - _get_stream_from_id and _get_all_stream_ids. These two helper methods help in preparation for adding stream and subscription property accessor methods in the next commit. Tests added.

This commit adds stream and subscription property accessor functions to limit the mutability of stream/subscription properties where it is not needed.

This commit replaces the direct use of stream_dict to access the 'name' property, with the stream accessor function introduced in the earlier commit. Since the new stream accessor function accesses subscribed streams, was subscribed streams and never-subscribed stream, the keyerror issue described in issue zulip#816 is resolved.

zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 27, 2023

theViz343 force-pushed the all-streams-accessor-functions branch from b6b236a to c444254 Compare July 27, 2023 13:47

neiljp mentioned this pull request Aug 1, 2023

Add support for all streams #1408

Open

18 tasks

neiljp reviewed Aug 1, 2023

View reviewed changes

neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Aug 1, 2023

theViz343 force-pushed the all-streams-accessor-functions branch from c444254 to d0068ff Compare August 2, 2023 11:30

theViz343 added 6 commits August 25, 2023 17:29

model: Improve typing of stream_dict.

1ced8ec

With the revamp of the Subscription TypedDict in an earlier commit, we use it to improve the typing of stream_dict in this commit. Tests and fixtures updated to support the new typing.

model: Add stream id helper methods.

311725a

This commit adds two helper methods - _get_stream_from_id and _get_all_stream_ids. These two helper methods help in preparation for adding stream and subscription property accessor methods in the next commit. Tests added.

model: Add stream and subscription property accessor functions.

1ce1e82

This commit adds stream and subscription property accessor functions to limit the mutability of stream/subscription properties where it is not needed.

theViz343 force-pushed the all-streams-accessor-functions branch from d0068ff to aa051b3 Compare August 28, 2023 21:42

theViz343 mentioned this pull request Sep 17, 2023

Add support for all streams minimal #1430

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for all streams (more general accessor methods) #1419

Add support for all streams (more general accessor methods) #1419

theViz343 commented Jul 27, 2023 •

edited

Loading

zulipbot commented Jul 27, 2023

neiljp left a comment

neiljp Aug 1, 2023

neiljp Aug 1, 2023

neiljp Aug 1, 2023

theViz343 Sep 9, 2023

neiljp Aug 1, 2023

neiljp Aug 1, 2023

neiljp Aug 1, 2023

		@@ -244,6 +233,21 @@ class Subscription(TypedDict):
		# in_home_view: bool # Replaced by is_muted in Zulip 2.1; still present in updates

Add support for all streams (more general accessor methods) #1419

Are you sure you want to change the base?

Add support for all streams (more general accessor methods) #1419

Conversation

theViz343 commented Jul 27, 2023 • edited Loading

What does this PR do, and why?

Outstanding aspect(s)

External discussion & connections

How did you test this?

Self-review checklist for each commit

Visual changes

zulipbot commented Jul 27, 2023

neiljp left a comment

Choose a reason for hiding this comment

neiljp Aug 1, 2023

Choose a reason for hiding this comment

neiljp Aug 1, 2023

Choose a reason for hiding this comment

neiljp Aug 1, 2023

Choose a reason for hiding this comment

theViz343 Sep 9, 2023

Choose a reason for hiding this comment

neiljp Aug 1, 2023

Choose a reason for hiding this comment

neiljp Aug 1, 2023

Choose a reason for hiding this comment

neiljp Aug 1, 2023

Choose a reason for hiding this comment

theViz343 commented Jul 27, 2023 •

edited

Loading