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.

Sign up for GitHub

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

Jump to bottom

[WIP] Migrate message['subject'] to message['topic'] #1236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[WIP] Migrate message['subject'] to message['topic'] #1236

wants to merge 1 commit into from

Conversation

mounilKshah
Copy link
Collaborator

What does this PR do?
This commit migrates the ZT to newer API elements. Here, the migration is from message['subject'] to message['topic'].
Partial fix for #965

Discussion:
#zulip-terminal>Update to modern API elements (support v2.1) #T965

Tested?

  • Manually
  • Existing tests (adapted, if necessary)
  • New tests added (for any new behavior)
  • Passed linting & tests (each commit)

Commit flow

  • First commit updates message element from subject to topic
  • Second commit refactors the tests to accommodate this migration

Notes & Questions

@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Jun 23, 2022
@mounilKshah mounilKshah added PR needs review PR requires feedback to proceed area: refactoring labels Jun 23, 2022
@zulipbot
Copy link
Member

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

neiljp
neiljp reviewed Jun 23, 2022
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@mounilKshah You have the right ideas here, but as per my stream feedback, it's good to keep code changes and associated test changes in the same commit.

In addition, you've got multiple changes here, which ideally would be in different commits. Part of that is that the main subject->topic migration is split into Message and Composition parts, ie. what we receive from the server vs what a user sends. However, the other aspect are API changes that have similar names but haven't necessarily changed, or that are adjacent. It's fine to make these changes, but it would be cleaner to see each change in a separate commit.

Each API change may be in a different version, so that opens up the scope for writing differently about the change, when it occurred, etc. in the commit text body.

zulipterminal/api_types.py Outdated
@@ -21,8 +21,8 @@ class PrivateComposition(TypedDict):
class StreamComposition(TypedDict):
type: Literal["stream"]
content: str
to: str # stream name # TODO: Migrate to using int (stream id)
subject: str # TODO: Migrate to using topic
to: Optional[int]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a separate migration. It may be OK in this PR, but certainly in another commit.

zulipterminal/api_types.py Outdated
to: str # stream name # TODO: Migrate to using int (stream id)
subject: str # TODO: Migrate to using topic
to: Optional[int]
topic: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

As per a comment in the stream, it may be cleaner to separate the compose and message parts of the topic migration into separate commits (with associated test changes in each).

zulipterminal/api_types.py Outdated
Comment on lines 37 to 39
subject: str
# Changing subject -> topic as per the migration in Zulip 2.0
topic: str # Only for stream msgs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest we comment out the subject, with a trailing comment referring to the migration.
In an ideal world that would mean using subject would cause a type error, but currently this TypedDict has total=False, so that won't happen (yet).

zulipterminal/api_types.py Outdated
@@ -53,7 +55,7 @@ class Message(TypedDict, total=False):
avatar_url: str
content_type: str
match_content: str # If keyword search specified in narrow params.
match_subject: str # If keyword search specified in narrow params.
match_topic: str # If keyword search specified in narrow params.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have support for this migration?

Copy link
Collaborator Author

@mounilKshah mounilKshah Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No. As subject was changed to topic, I felt it would be good to rename this accordingly. But as the API is still the same, I'll retain the original names for this and orig_topic

zulipterminal/api_types.py Outdated
Comment on lines 150 to 151
orig_topic: str
topic: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have support for this migration in the API?

@neiljp neiljp mentioned this pull request Jun 24, 2022
Merged
4 tasks
@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 24, 2022
@mounilKshah
refactor: Migrate subject -> topic.
57df32e 
This commit contains migration for message["subject"]
to message ["topic"] as per the modern API with the oldest
support being v2.1. It also contains the updated tests for
the migration.
@mounilKshah mounilKshah added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 25, 2022
@neiljp
Copy link
Collaborator

neiljp commented Jul 25, 2022

@mounilKshah I left some answers in the stream; do you need further feedback before the tests are updated?

@neiljp neiljp added api migrations area: API spec and removed PR needs review PR requires feedback to proceed labels Aug 28, 2022
@neiljp
Copy link
Collaborator

neiljp commented Aug 28, 2022

@mounilKshah Thanks for investigating this!

I left a message in the stream, but for other readers also:

  • The API relating to migration of the subject field to topic is the only (or almost only) renaming of this kind which it seems has not yet occurred.
  • Since the only current commit in this PR relates to this, I propose we hold off on this migration, since this changes our internal data structures unnecessarily.
  • Another part related to this migration is when sending messages via Composition objects, which supported topic since 2.0.0; let's redirect that effort into another PR.

@zulipbot
Copy link
Member

Heads up @mounilKshah, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
api migrations area: API spec area: refactoring has conflicts size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mounilKshah @zulipbot @neiljp