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.

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

External hotkey commands: tagging, linting and checking sync #1498

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/config/test_keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def test_display_key_for_urwid_key(urwid_key: str, display_key: str) -> None:


COMMAND_TO_DISPLAY_KEYS = [
("NEXT_LINE", ["Down", "Ctrl n"]),
("SEND_MESSAGE", ["Ctrl d", "Meta Enter"]),
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation behind this change?

Copy link

@zormit zormit Jun 3, 2024

Choose a reason for hiding this comment

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

I'd have the same question :) (just reposting this, because it's now my turn to review)

Copy link

Choose a reason for hiding this comment

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

Also, partially answering it myself. The commit message gives it away: The change seems to aim to

not use readline hotkeys [in tests]

but I'm not sure why. Is it because we can not rely on the existence of readline shortcuts due to upstream changes? That seems reasonable to me. In general, it could be helpful to add the motivation for the goal to the commit message :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot to get back regarding this, @zormit, my apologies.
Sashank and I had talked about this, so I hadn't added an answer here for his initial question.

Yes, you're spot on.
Thank you for the pointer. You've worded it pretty well, I did not think of it in those terms.

("TOGGLE_STAR_STATUS", ["Ctrl s", "*"]),
("ALL_PM", ["P"]),
]
Expand Down
72 changes: 68 additions & 4 deletions tools/lint-hotkeys
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,92 @@ from zulipterminal.config.keys import (
KEY_BINDINGS,
display_keys_for_command,
)
from zulipterminal.config.regexes import (
REGEX_READLINE_COMMANDS,
REGEX_TERMINAL_COMMANDS,
)


KEYS_FILE = (
Path(__file__).resolve().parent.parent / "zulipterminal" / "config" / "keys.py"
)
# absolute path to zulip-terminal
ROOT_DIRECTORY = Path(__file__).resolve().parent.parent

# absolute path to zulip-terminal/zulipterminal to be passed as parameter
ZULIPTERMINAL = ROOT_DIRECTORY / "zulipterminal"

KEYS_FILE = ZULIPTERMINAL / "config" / "keys.py"
KEYS_FILE_NAME = KEYS_FILE.name
OUTPUT_FILE = Path(__file__).resolve().parent.parent / "docs" / "hotkeys.md"
OUTPUT_FILE = ROOT_DIRECTORY / "docs" / "hotkeys.md"
OUTPUT_FILE_NAME = OUTPUT_FILE.name
SCRIPT_NAME = PurePath(__file__).name
HELP_TEXT_STYLE = re.compile(r"^[a-zA-Z /()',&@#:_-]*$")

# Exclude keys from duplicate keys checking
KEYS_TO_EXCLUDE = ["q", "e", "m", "r"]

# Exclude files from being checked for external command usage
EXCLUDED_FILES = [
KEYS_FILE,
ZULIPTERMINAL / "config" / "regexes.py",
ZULIPTERMINAL / "cli" / "run.py",
]


def main(fix: bool) -> None:
lint_all_external_commands()
if fix:
generate_hotkeys_file()
else:
lint_hotkeys_file()


def lint_all_external_commands() -> None:
lint_external_commands_by_type(
regex_pattern=REGEX_READLINE_COMMANDS,
command_type="Urwid Readline",
suffix="READLINE_SUFFIX",
)
lint_external_commands_by_type(
regex_pattern=REGEX_TERMINAL_COMMANDS,
command_type="General terminal",
suffix="GENERAL_TERMINAL_SUFFIX",
)
print("All external commands have been linted successfully.")


def lint_external_commands_by_type(
regex_pattern: str, command_type: str, suffix: str
) -> None:
"""
Lint src directory for the usage of external commands
in the codebase by checking for their regex
"""
error_flag = 0
for file_path in ZULIPTERMINAL.rglob("*.py"):
if file_path in EXCLUDED_FILES:
continue
with file_path.open() as f:
contents = f.read()
regex_matches = re.finditer(regex_pattern, contents)
suffix_matches = re.finditer(suffix, contents)
count_matches = sum(1 for _ in regex_matches) + sum(
1 for _ in suffix_matches
)
if count_matches > 0:
print(
f"{file_path.name} contains {count_matches} mentions of {command_type} commands."
)
error_flag = 1
if error_flag == 1:
print(
f"{command_type} commands are not intended for direct use or modification."
)
print(
f"Please refer to {KEYS_FILE_NAME} for identifying the {command_type} commands."
)
print("Rerun this command after removing the usage of external commands.")
sys.exit(error_flag)
Comment on lines +74 to +97
Copy link
Member

Choose a reason for hiding this comment

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

Could we adjust this approach to include the line number at which it was found along with the file name?



def lint_hotkeys_file() -> None:
"""
Lint KEYS_FILE for key description, then compare if in sync with
Expand Down
41 changes: 41 additions & 0 deletions zulipterminal/cli/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@

import requests
from urwid import display_common, set_encoding
from urwid_readline import ReadlineEdit

from zulipterminal.api_types import ServerSettings
from zulipterminal.config.keys import KEY_BINDINGS, READLINE_SUFFIX
from zulipterminal.config.themes import (
ThemeError,
aliased_themes,
Expand Down Expand Up @@ -392,6 +394,34 @@ def list_themes() -> str:
)


class ReadlineShortcutError(Exception):
pass


def check_readline_shortcuts_availability() -> None:
readline_edit = ReadlineEdit()
commands_to_exclude = ["PREV_LINE" + READLINE_SUFFIX, "NEXT_LINE" + READLINE_SUFFIX]
filtered_commands = [
command
for command in KEY_BINDINGS
if command.endswith(READLINE_SUFFIX) and command not in commands_to_exclude
]

missing_keys = []
for command in filtered_commands:
for key in KEY_BINDINGS[command]["keys"]:
if key not in readline_edit.keymap:
key_missing_error = (
f'Key "{key}" for command "{KEY_BINDINGS[command]["help_text"]}" '
f"is not found."
)
missing_keys.append(key_missing_error)

if missing_keys:
error_message = "\n".join(missing_keys)
raise ReadlineShortcutError(error_message)


def main(options: Optional[List[str]] = None) -> None:
"""
Launch Zulip Terminal.
Expand Down Expand Up @@ -566,6 +596,17 @@ def print_setting(setting: str, data: SettingData, suffix: str = "") -> None:
for setting, valid_boolean_values in VALID_BOOLEAN_SETTINGS.items():
boolean_settings[setting] = zterm[setting].value == valid_boolean_values[0]

try:
check_readline_shortcuts_availability()
except ReadlineShortcutError as e:
print(
"\nThe following readline shortcuts "
+ "are missing in urwid_readline's keymap.\n"
+ str(e)
+ "\n",
file=sys.stderr,
)

Controller(
config_file=zuliprc_path,
maximum_footlinks=maximum_footlinks,
Expand Down
40 changes: 22 additions & 18 deletions zulipterminal/config/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
)


READLINE_SUFFIX = "_READLINE"
GENERAL_TERMINAL_SUFFIX = "_GENERAL_TERMINAL"


class KeyBinding(TypedDict):
keys: List[str]
help_text: str
Expand Down Expand Up @@ -305,12 +309,12 @@ class KeyBinding(TypedDict):
'excluded_from_random_tips': True,
'key_category': 'stream_list',
},
'REDRAW': {
'REDRAW' + GENERAL_TERMINAL_SUFFIX: {
'keys': ['ctrl l'],
'help_text': 'Redraw screen',
'key_category': 'general',
},
'QUIT': {
'QUIT' + GENERAL_TERMINAL_SUFFIX: {
'keys': ['ctrl c'],
'help_text': 'Quit',
'key_category': 'general',
Expand All @@ -320,82 +324,82 @@ class KeyBinding(TypedDict):
'help_text': 'Show/hide user information (from users list)',
'key_category': 'general',
},
'BEGINNING_OF_LINE': {
'BEGINNING_OF_LINE' + READLINE_SUFFIX: {
'keys': ['ctrl a', 'home'],
'help_text': 'Start of line',
'key_category': 'editor_navigation',
},
'END_OF_LINE': {
'END_OF_LINE' + READLINE_SUFFIX: {
'keys': ['ctrl e', 'end'],
'help_text': 'End of line',
'key_category': 'editor_navigation',
},
'ONE_WORD_BACKWARD': {
'ONE_WORD_BACKWARD' + READLINE_SUFFIX: {
'keys': ['meta b', 'shift left'],
'help_text': 'Start of current or previous word',
'key_category': 'editor_navigation',
},
'ONE_WORD_FORWARD': {
'ONE_WORD_FORWARD' + READLINE_SUFFIX: {
'keys': ['meta f', 'shift right'],
'help_text': 'Start of next word',
'key_category': 'editor_navigation',
},
'PREV_LINE': {
'PREV_LINE' + READLINE_SUFFIX: {
'keys': ['up', 'ctrl p'],
'help_text': 'Previous line',
'key_category': 'editor_navigation',
},
'NEXT_LINE': {
'NEXT_LINE' + READLINE_SUFFIX: {
'keys': ['down', 'ctrl n'],
'help_text': 'Next line',
'key_category': 'editor_navigation',
},
'UNDO_LAST_ACTION': {
'UNDO_LAST_ACTION' + READLINE_SUFFIX: {
'keys': ['ctrl _'],
'help_text': 'Undo last action',
'key_category': 'editor_text_manipulation',
},
'CLEAR_MESSAGE': {
'CLEAR_MESSAGE' + READLINE_SUFFIX: {
'keys': ['ctrl l'],
'help_text': 'Clear text box',
'key_category': 'editor_text_manipulation',
},
'CUT_TO_END_OF_LINE': {
'CUT_TO_END_OF_LINE' + READLINE_SUFFIX: {
'keys': ['ctrl k'],
'help_text': 'Cut forwards to the end of the line',
'key_category': 'editor_text_manipulation',
},
'CUT_TO_START_OF_LINE': {
'CUT_TO_START_OF_LINE' + READLINE_SUFFIX: {
'keys': ['ctrl u'],
'help_text': 'Cut backwards to the start of the line',
'key_category': 'editor_text_manipulation',
},
'CUT_TO_END_OF_WORD': {
'CUT_TO_END_OF_WORD' + READLINE_SUFFIX: {
'keys': ['meta d'],
'help_text': 'Cut forwards to the end of the current word',
'key_category': 'editor_text_manipulation',
},
'CUT_TO_START_OF_WORD': {
'CUT_TO_START_OF_WORD' + READLINE_SUFFIX: {
'keys': ['ctrl w', 'meta backspace'],
'help_text': 'Cut backwards to the start of the current word',
'key_category': 'editor_text_manipulation',
},
'CUT_WHOLE_LINE': {
'CUT_WHOLE_LINE' + READLINE_SUFFIX: {
'keys': ['meta x'],
'help_text': 'Cut the current line',
'key_category': 'editor_text_manipulation',
},
'PASTE_LAST_CUT': {
'PASTE_LAST_CUT' + READLINE_SUFFIX: {
'keys': ['ctrl y'],
'help_text': 'Paste last cut section',
'key_category': 'editor_text_manipulation',
},
'DELETE_LAST_CHARACTER': {
'DELETE_LAST_CHARACTER' + READLINE_SUFFIX: {
'keys': ['ctrl h'],
'help_text': 'Delete previous character',
'key_category': 'editor_text_manipulation',
},
'TRANSPOSE_CHARACTERS': {
'TRANSPOSE_CHARACTERS' + READLINE_SUFFIX: {
'keys': ['ctrl t'],
'help_text': 'Swap with previous character',
'key_category': 'editor_text_manipulation',
Expand Down
9 changes: 9 additions & 0 deletions zulipterminal/config/regexes.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@


# (*) Stream and topic regexes
from zulipterminal.config.keys import GENERAL_TERMINAL_SUFFIX, READLINE_SUFFIX


REGEX_STREAM_NAME = r"([^*>]+)"
REGEX_TOPIC_NAME = r"([^*]*)"

Expand Down Expand Up @@ -46,3 +49,9 @@

# Example: 6-test-stream
REGEX_INTERNAL_LINK_STREAM_ID = r"^[0-9]+-"


# Example: UNDO_LAST_ACTION_READLINE
REGEX_READLINE_COMMANDS = rf"([A-Z_]+{READLINE_SUFFIX})"
# Example: REDRAW_GENERAL_TERMINAL
REGEX_TERMINAL_COMMANDS = rf"^.*([A-Z_]+{GENERAL_TERMINAL_SUFFIX})"
Loading