zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

Published 20 hours ago • verified publisher icon zulip

Support server-customized presence parameters.

Open rsashank opened this issue 1 year ago • 2 comments

Fixes #1421 Instead of hard-coded constants, uses server provided values, falling back to the original constants when server version is lower.

What does this PR do, and why?

Outstanding aspect(s)

  • [ ]

External discussion & connections

  • [ ] Discussed in #zulip-terminal in topic
  • [x] Fully fixes #1421
  • [ ] 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 #

How did you test this?

  • [ ] Manually - Behavioral changes
  • [ ] Manually - Visual changes
  • [x] Adapting existing automated tests
  • [x] 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

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

Visual changes

rsashank avatar Jan 21 '24 15:01 rsashank

@rsashank Is this ready for review? I know you have other PRs ongoing, but just wanted to check since there was no label.

I noticed you handled only one part of the issue so far. For moving this forward, some of my feedback on #1448 may also be helpful.

neiljp avatar Feb 22 '24 03:02 neiljp

@neiljp, I had tests going on this month, so couldn't get back to this.

Anyways, I've updated the PR, addressed the other part of the issue. I believe it's ready for review now.

@zulipbot add "PR needs review"

rsashank avatar Feb 26 '24 19:02 rsashank

@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"

rsashank avatar Apr 14 '24 21:04 rsashank

@zulipbot remove "PR awaiting update" @zulipbot add "PR needs review"

rsashank avatar Apr 15 '24 02:04 rsashank

I was just updating this right now; looks like I won't need to push changes from my end anymore 😅. Sorry for any added hassle. It now makes sense; initially, I thought it was a refactor since it wasn't altering functionality per se, but having it customizable directly from the server does seem like added functionality. It's great to have that clear, my bad.

Thanks for the review again!

image

rsashank avatar Apr 17 '24 22:04 rsashank

@rsashank Thanks - I should have specified within a certain timeframe instead :)

Anyhow, take a look at your branch vs this by fetching the pull request (see in /tools/) and use git range-diff to compare the branch :)

I'm merging now :tada:

neiljp avatar Apr 17 '24 22:04 neiljp