Generate hotkey using script #926

mkp6781 · 2021-02-17T10:52:11Z

Fixes #126

zulipbot · 2021-02-17T10:52:18Z

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

tools/generate_hotkey.py

neiljp

@mkp6781 This looks like a good start - I left a few comments 👍

docs/hotkey.md

tools/generate_hotkey.py

neiljp

@mkp6781 Just had another quick look - the order of sections appears to have changed? This may change as you finalize the data structures from keys.py in any case.

README.md

tools/generate_hotkey.py

mkp6781 · 2021-02-23T11:29:25Z

To generate the hotkey section exactly as before, a few minor adjustments may be needed in the keys.py file.

tools/generate_hotkey.py

neiljp · 2021-02-24T17:45:44Z

tools/generate_hotkey.py

+    }
+    for category in HELP_CATEGORIES.keys()
+}
+OUTPUT_FILE = "docs/hotkey.md"


This fails if not run as tools/... - you can get the absolute path of this __file__ and look at the relative path from there. I'd suggest using pathlib in python 3.

tools/generate_hotkey.py

neiljp · 2021-02-24T19:24:29Z

@mkp6781 I'm excited to have this done! 👍

We should perhaps adjust the generated file to be plural (...keys.md), and I'm not sure why we have the title capitalized; I'm not sure we do in other documentation?

Regarding your point about the difference between the files, I'd suggest adding an intermediate commit with the extra/adjusted entries.

Updated links to hot keys section in setup.py and README. Removed repeated navigation tips at the end of hot keys section.

neiljp

@mkp6781 I hope you can already see the readability improvements from the last few iterations?

There are a few more updates we need based on the extra commit, and there are some changes to the generation script which could simplify that even further which you may be able to identify from the previous iterations. The plural form looks better, but let's change the script name to match too?

neiljp · 2021-02-25T19:20:13Z

zulipterminal/config/keys.py

@@ -22,7 +22,7 @@ class KeyBinding(TypedDict, total=False):
        'key_category': 'general',
    }),
    ('ABOUT', {
-        'keys': ['meta ?'],


These changes break the application - while these are used by the help docs, urwid requires them to be in a certain format too. So, if you plan to capitalize them (do modifiers need that?) then that'll be necessary in the script instead.

I'd suggest you leave them as they are for now - I think the keys.py text is likely more up to date for the keys it has, but is missing those extra keys you've identified below.

Note that we don't support ctrl d, as it's currently the sending key. We've gone back and forth on this issue previously in the stream.

neiljp · 2021-02-25T19:39:20Z

tools/generate_hotkey.py

+            various_key_combinations = []
+            for key_combination in key_combinations_list:
+                new_key_list = key_combination.split()
+                new_key_list = [
+                    "<kbd>" + key + "</kbd>"
+                    for key in new_key_list
+                ]
+                new_key_list = " + ".join(new_key_list)
+                various_key_combinations.append(new_key_list)
+            various_key_combinations = " / ".join(various_key_combinations)


This is already a lot clearer, but you may be able to see how we can simplify this even further.

neiljp · 2021-02-25T19:43:10Z

tools/generate_hotkey.py

+    }
+    for category in HELP_CATEGORIES.keys()
+}
+OUTPUT_FILE = Path(__file__).parent.parent.joinpath('docs', 'hotkeys.md')


This is a good first step to use pathlib 👍

However, this doesn't also work from the tools directory (tools/generate_hotkey.py vs ./generate_hotkey.py vs zulipterminal/tools/generate_hotkey.py vs...) - you need the absolute path. Pathlib also allows clever use of / to join paths, which you might enjoy too, instead of joinpath.

neiljp · 2021-02-25T19:49:36Z

tools/generate_hotkey.py

+    mdFile.write("<!--- Generated automatically by tools/generate_hotkey.py -->\n"
+                 + "<!--- Do not modify -->\n"
+                 + "\n# Hot Keys\n")


For consecutive strings this can be even tidier by skipping the + :)

neiljp · 2021-02-25T19:50:00Z

tools/generate_hotkey.py

+            for key_combination in key_combinations_list:
+                new_key_list = key_combination.split()
+                new_key_list = [
+                    "<kbd>" + key + "</kbd>"


neiljp · 2021-02-25T20:03:10Z

tools/generate_hotkey.py

+OUTPUT_FILE = Path(__file__).parent.parent.joinpath('docs', 'hotkeys.md')
+
+with open(OUTPUT_FILE,"w") as mdFile:
+    mdFile.write("<!--- Generated automatically by tools/generate_hotkey.py -->\n"


Since we are not using any variables here, do we need an fstring?

The filename itself is a variable, so we can avoid adjusting this text (or part of it) when the script runs.

I do not quite follow you here. Isn't the filename a constant with the root directory as zulipterminal. A similar case can be seen in convert-unicode-emoji-data.

Noticed the change in convert-unicode-emoji-data, I've made the same change 👍🏼

Made minor changes to key_combinations and help_text in `KEY_BINDINGS` and `HELP_CATEGORIES`. When automating generation of this section in a later commit, this refactor enables exact matching of output keys with current Hot Keys section in hotkeys.md.

mkp6781 · 2021-02-28T06:25:20Z

@zulipbot add "PR needs review"

Generates markdown file hotkeys.md using `KEY_BINDINGS` and `HELP_CATEGORIES` from `keys.py`. This ensures that changes to command keys are reflected automatically in the documentation when the user runs the `generate_hotkeys.py` script. Fixes zulip#126.

zulipbot · 2021-03-02T02:47:59Z

Heads up @mkp6781, we just merged some commits that conflict with the changes your 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.

neiljp · 2021-03-02T02:56:37Z

@mkp6781 Thanks for this - this exposes various cases where we weren't keeping the documents synchronized, and will make updating categories and text much easier 👍 I've just merged this with a few adjustments to the first commit due to broken links, chose to exclude/update various key data from the second, and merged the third as it was 🎉

zulipbot added size: XL [Automatic label added by zulipbot] area: documentation Requires changes in documentation area: hotkeys area: infrastructure Project infrastructure high priority should be done as soon as possible PR needs review PR requires feedback to proceed labels Feb 17, 2021

Abhirup-99 reviewed Feb 17, 2021

View reviewed changes

tools/generate_hotkey.py Outdated Show resolved Hide resolved

neiljp reviewed Feb 17, 2021

View reviewed changes

docs/hotkey.md Outdated Show resolved Hide resolved

tools/generate_hotkey.py Outdated Show resolved Hide resolved

tools/generate_hotkey.py Outdated Show resolved Hide resolved

tools/generate_hotkey.py Outdated Show resolved Hide resolved

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 17, 2021

mkp6781 force-pushed the generate_hotkey_using_script branch 2 times, most recently from eb072a3 to 5901013 Compare February 18, 2021 07:20

neiljp reviewed Feb 18, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

tools/generate_hotkey.py Outdated Show resolved Hide resolved

tools/generate_hotkey.py Outdated Show resolved Hide resolved

mkp6781 force-pushed the generate_hotkey_using_script branch 2 times, most recently from 79de909 to 0c8ed4b Compare February 23, 2021 11:00

neiljp reviewed Feb 24, 2021

View reviewed changes

neiljp added this to the Next Release milestone Feb 24, 2021

README/hotkeys: Move HotKeys section to docs/hotkeys.

20ade6e

Updated links to hot keys section in setup.py and README. Removed repeated navigation tips at the end of hot keys section.

mkp6781 force-pushed the generate_hotkey_using_script branch from 0c8ed4b to 040f97c Compare February 25, 2021 18:53

neiljp reviewed Feb 25, 2021

View reviewed changes

mkp6781 force-pushed the generate_hotkey_using_script branch from 040f97c to 17394b9 Compare February 26, 2021 07:31

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

mkp6781 force-pushed the generate_hotkey_using_script branch from 17394b9 to b55adac Compare March 1, 2021 14:54

zulipbot added the has conflicts label Mar 2, 2021

neiljp closed this Mar 2, 2021

Ezio-Sarthak mentioned this pull request Mar 29, 2021

feature: Topic muting #914

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate hotkey using script #926

Generate hotkey using script #926

mkp6781 commented Feb 17, 2021 •

edited

Loading

zulipbot commented Feb 17, 2021

neiljp left a comment

neiljp left a comment

mkp6781 commented Feb 23, 2021

neiljp Feb 24, 2021

neiljp commented Feb 24, 2021

neiljp left a comment

neiljp Feb 25, 2021

neiljp Feb 25, 2021

neiljp Feb 25, 2021

neiljp Feb 25, 2021

neiljp Feb 25, 2021

neiljp Feb 25, 2021

mkp6781 Feb 26, 2021

neiljp Feb 26, 2021

mkp6781 Feb 28, 2021

mkp6781 Mar 1, 2021

mkp6781 commented Feb 28, 2021

zulipbot commented Mar 2, 2021

neiljp commented Mar 2, 2021

Generate hotkey using script #926

Generate hotkey using script #926

Conversation

mkp6781 commented Feb 17, 2021 • edited Loading

zulipbot commented Feb 17, 2021

neiljp left a comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

mkp6781 commented Feb 23, 2021

Choose a reason for hiding this comment

neiljp commented Feb 24, 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

Choose a reason for hiding this comment

mkp6781 commented Feb 28, 2021

zulipbot commented Mar 2, 2021

neiljp commented Mar 2, 2021

mkp6781 commented Feb 17, 2021 •

edited

Loading