footlinks: Add threshold configuration for footlinks. #841

Abhirup-99 · 2020-12-05T16:20:11Z

This enables user to configure footlinks threshold settings
using footlinks_threshold in zuliprc, retiring the previous
footlinks. Tests have been updated to reflect the change.
Fixes #773.

Abhirup-99 · 2021-01-10T09:23:52Z

@neiljp pr is updated, please take a look now.

neiljp

@Abhirup-99 This seems to work well - and I'd forgotten how many places we use footlinks now!

Other than the inline comments, some other general thoughts:

You have updated tests, but it would be great to see one for the new behavior, unless I've missed this being covered - preferably at least testing how the controller parameter affects the number of shown footlinks, and potentially that the new value gets passed in from the config file?
You've removed the functionality we previously had for that config option, which we have in 0.5.2; it'd be useful to allow the old option to be used, perhaps only as an alternative, but without allowing both in the file at the same time.
The README doesn't document the new/alternative option
We could add a FAQ entry about enabling/disabling/maximum footlinks, particularly in terms of how the old/new option interacts?

tests/cli/test_run.py

zulipterminal/cli/run.py

tests/core/test_core.py

Abhirup-99 · 2021-01-15T17:52:03Z

@zulipbot add "PR needs review".

neiljp

@Abhirup-99 This has become quite complex for a 'good first issue', so thanks for sticking with it!
(most notably to handle the overlapping old and new config options, as well as the tests with configs)

The parameterized_zuliprc looks to be really useful for adding other tests for our config handling later 👍

We should be able to simplify the tests later if we refactor main, which you might have mentioned in the stream?

tests/cli/test_run.py

neiljp · 2021-01-16T22:49:05Z

tests/cli/test_run.py

+        "notify": "enabled",
+        "color-depth": "256"
+    }])
+def test_successful_main_function_with_config(


This test is with a config file but specifying the default values?

Yes, just a default value check, we can remove it though. Was writing a bunch of tests, so thought one which checks the default flow would also be useful.

I think having the set of defaults passing would be a good test case, but we could integrate it with the similar pre-existing one, perhaps? I'd have to take another look.

Ok sure, but I have updated it to make it more of a successful main completion with various variations of maximum-footlinks and footlinks values.

neiljp · 2021-01-16T22:50:47Z

tests/cli/test_run.py

+                                             ):
+    modified_zuliprc = parameterized_zuliprc(minimal_zuliprc, zulip_config)
+    mocker.patch('zulipterminal.core.Controller.__init__',
+                 side_effect=SystemExit)


Does it matter that this has a side effect?

Yes, without the side effect, the functions would never terminate, as it would go one with Controller initialization.

I think we have a number of tests which might not terminate on failing; I'd like to resolve that, but doesn't the validation failure happen before the Controller init?

The control would finish in each case whether it goes to completion with the SystemExit in Controller init or it throws errors and stops, or it stops with SystemExit in Controller init and the test fails due to the assert statements after the completion. The SystemExit are the safety nets that would ensure that in any of the above cases the control finishes.

I have updated the logic to mock the calls now, rather than using SystemExit.

tests/cli/test_run.py

tests/ui/test_ui_tools.py

zulipterminal/cli/run.py

tests/cli/test_run.py

Abhirup-99 · 2021-01-17T22:11:22Z

@zulipbot add "PR needs review".

neiljp

@Abhirup-99 The parametrized_zuliprc is great 👍

To clarify what I said before, we want disabled=0 and enabled=3 for the new maximum levels (not 1000). If the user wants other values then they can use maximum-footlinks=[integer>=0]. Perhaps the maximum=3 was confused with enabled=3 in the previous version?

Regarding the refactor commit, I was referring to extracting something like exit_with_error("some text"). This would simplify the code slightly (include some red color, exit appropriately), but also ease testing as you can

mock it
assert on the text passed in
check it raises

For ensuring the Controller __init__ doesn't happen, if we mock it without a side-effect then it does nothing? We can assert that it does or doesn't get called, too, if we're supposed to exit before that point.

tests/cli/test_run.py

zulipterminal/cli/run.py

neiljp · 2021-01-20T07:19:49Z

zulipterminal/cli/run.py

@@ -285,6 +286,19 @@ def main(options: Optional[List[str]]=None) -> None:
        else:
            theme_to_use = zterm['theme']

+        ZULIPRC_CONFIG = 'in zuliprc file'


I'd prefer we didn't rely on testing against this - this is intended for output purposes.

Perhaps we can move the validation into parse_zuliprc?

But parse_zuliprc is specifically for parsing the zuliprc file, rather than validation. All other validations take place in the main function.

neiljp · 2021-01-20T07:24:15Z

zulipterminal/cli/run.py

+                # Random Large Figure which practically
+                # wouldn't be there in the messages
+                maximum_footlinks = 1000


This should be 3; see my general feedback.

Ahem. After running with the non-PR dev version quite a bit, I see lots of footlinks in some cases, so 1000 seems like a reasonable limit after all. Ideally we wouldn't use a fixed high number but something more programmatic, eg rather than comparing x to 1000 in a conditional we could wrap it and have a bool be True or False in this case, and dependent on the number otherwise.

neiljp · 2021-01-20T07:26:34Z

zulipterminal/cli/run.py

+        if zterm['footlinks'][1] == ZULIPRC_CONFIG:
+            print("   footlinks setting '{}' specified {}."
+                  .format(*zterm['footlinks']))
+        else:
+            print("   maximum footlinks setting '{}' specified {}."
+                  .format(*zterm['maximum-footlinks']))


We output this as just a maximum value in the about popup; we should make them consistent.

We could output the maximum and append an element to the last part of the zterm tuple to indicate it came from the footlinks option?

That could be done, but then it would force another if clause during the traversing of zterm, and again another to check the length of the tuple during printing. Wouldn't a bit of hard-coded values and an if-else during printing be better than that approach?

Abhirup-99 · 2021-01-20T18:50:21Z

@zulipbot add "PR needs review".

Abhirup-99 · 2021-01-20T21:48:00Z

@zulipbot remove "PR needs update"

neiljp

@Abhirup-99 This is looking really ready :)

These should all be fairly straightforward changes, but let me know if not 👍

zulipterminal/cli/run.py

tests/cli/test_run.py

README.md

tests/cli/test_run.py

Abhirup-99 · 2021-02-01T17:14:13Z

@zulipbot add "PR needs review".

Abhirup-99 · 2021-02-01T17:14:29Z

@zulipbot remove "PR needs update"

This enables the user to configure the maximum footlinks in zuliprc. The change is backward compatible with footlinks introduced in 0.5.2.README updated. Fixes zulip#773.

neiljp · 2021-02-02T04:50:47Z

@Abhirup-99 Thanks for your extended work on this - I've just merged this manually as 0eeb6ea 🎉

As we discussed this does slightly change the previous meaning/behavior, which will need addressing in the release notes, or we could adjust later to be more backwards compatible.

I adjusted the general layout in various places including in the tests and made a small simplification, removed some apparent holdover code from the previous commit we already merged and clarified the commit text. I'd suggest looking into git range-diff if you haven't already for seeing what's different, though this is simple enough for a git diff between branches to see what I tweaked.

Abhirup-99 force-pushed the add_footlink_threshold branch from a38e3fa to 9ff5ab2 Compare December 9, 2020 15:36

Abhirup-99 force-pushed the add_footlink_threshold branch from 9ff5ab2 to 298922e Compare December 17, 2020 11:20

neiljp added the PR needs review PR requires feedback to proceed label Dec 21, 2020

neiljp added the has conflicts label Jan 7, 2021

Abhirup-99 force-pushed the add_footlink_threshold branch from 298922e to c9eb86f Compare January 10, 2021 09:21

neiljp removed the has conflicts label Jan 12, 2021

neiljp reviewed Jan 12, 2021

View reviewed changes

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 Jan 12, 2021

Abhirup-99 force-pushed the add_footlink_threshold branch 7 times, most recently from 9281799 to 77841d6 Compare January 14, 2021 16:50

neiljp mentioned this pull request Jan 15, 2021

[WIP] footlinks: Add threshold configuration for footlinks. #780

Closed

zulipbot added the PR needs review PR requires feedback to proceed label Jan 15, 2021

neiljp reviewed Jan 16, 2021

View reviewed changes

neiljp removed the PR needs review PR requires feedback to proceed label Jan 16, 2021

Abhirup-99 force-pushed the add_footlink_threshold branch from 77841d6 to 8ae61f0 Compare January 17, 2021 22:04

zulipbot added the size: XL [Automatic label added by zulipbot] label Jan 17, 2021

zulipbot added the PR needs review PR requires feedback to proceed label Jan 17, 2021

neiljp reviewed Jan 20, 2021

View reviewed changes

Abhirup-99 force-pushed the add_footlink_threshold branch 3 times, most recently from 495f20c to e16e1f1 Compare January 20, 2021 18:21

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jan 20, 2021

neiljp mentioned this pull request Jan 27, 2021

Refactor run.py with DEFAULT_SETTINGS & simple error exits #893

Merged

Abhirup-99 force-pushed the add_footlink_threshold branch 2 times, most recently from b7cd789 to 9e8e23a Compare January 28, 2021 06:26

neiljp added this to the Release after next milestone Jan 28, 2021

Base automatically changed from master to main January 30, 2021 20:31

Abhirup-99 force-pushed the add_footlink_threshold branch 2 times, most recently from 9e3fcea to 94b57ec Compare January 31, 2021 13:12

neiljp reviewed Feb 1, 2021

View reviewed changes

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 Feb 1, 2021

Abhirup-99 force-pushed the add_footlink_threshold branch 2 times, most recently from 6e8f299 to 50c9e8e Compare February 1, 2021 17:13

zulipbot added the PR needs review PR requires feedback to proceed label Feb 1, 2021

zulipbot removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Feb 1, 2021

Abhirup-99 force-pushed the add_footlink_threshold branch from 50c9e8e to 35bb784 Compare February 1, 2021 19:04

footlinks: Add threshold configuration for footlinks.

ec69179

This enables the user to configure the maximum footlinks in zuliprc. The change is backward compatible with footlinks introduced in 0.5.2.README updated. Fixes zulip#773.

Abhirup-99 force-pushed the add_footlink_threshold branch from 35bb784 to ec69179 Compare February 1, 2021 19:07

neiljp closed this Feb 2, 2021

Abhirup-99 deleted the add_footlink_threshold branch February 2, 2021 07:47

Abhirup-99 restored the add_footlink_threshold branch February 2, 2021 09:36

Abhirup-99 deleted the add_footlink_threshold branch February 2, 2021 09:55

neiljp added the area: config label Feb 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

footlinks: Add threshold configuration for footlinks. #841

footlinks: Add threshold configuration for footlinks. #841

Abhirup-99 commented Dec 5, 2020

Abhirup-99 commented Jan 10, 2021

neiljp left a comment

Abhirup-99 commented Jan 15, 2021

neiljp left a comment

neiljp Jan 16, 2021

Abhirup-99 Jan 17, 2021

neiljp Jan 19, 2021

Abhirup-99 Jan 19, 2021

neiljp Jan 16, 2021

Abhirup-99 Jan 17, 2021

neiljp Jan 19, 2021

Abhirup-99 Jan 19, 2021

Abhirup-99 Jan 27, 2021

Abhirup-99 commented Jan 17, 2021

neiljp left a comment

neiljp Jan 20, 2021

Abhirup-99 Jan 20, 2021

neiljp Jan 20, 2021

neiljp Jan 26, 2021

neiljp Jan 20, 2021

Abhirup-99 Jan 20, 2021 •

edited

Abhirup-99 commented Jan 20, 2021

Abhirup-99 commented Jan 20, 2021

neiljp left a comment

Abhirup-99 commented Feb 1, 2021

Abhirup-99 commented Feb 1, 2021

neiljp commented Feb 2, 2021

footlinks: Add threshold configuration for footlinks. #841

footlinks: Add threshold configuration for footlinks. #841

Conversation

Abhirup-99 commented Dec 5, 2020

Abhirup-99 commented Jan 10, 2021

neiljp left a comment

Choose a reason for hiding this comment

Abhirup-99 commented Jan 15, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abhirup-99 commented Jan 17, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abhirup-99 Jan 20, 2021 • edited

Choose a reason for hiding this comment

Abhirup-99 commented Jan 20, 2021

Abhirup-99 commented Jan 20, 2021

neiljp left a comment

Choose a reason for hiding this comment

Abhirup-99 commented Feb 1, 2021

Abhirup-99 commented Feb 1, 2021

neiljp commented Feb 2, 2021

Abhirup-99 Jan 20, 2021 •

edited