External hotkey commands: tagging, linting and checking sync #1498
Conversation
Niloth-p
added
area: hotkeys
PR needs review
|
Hello @zulip/server-hotkeys members, this pull request was labeled with the "area: hotkeys" label, so you may want to check it out! |
|
FWIW, I had put myself as a reviewer here earlier, but I'll wait for a peer review by @rsashank before looking at this one in more detail. Briefly looking at it (and maybe this has been discussed, I didn't look far), I'm wondering whether the "suffix-based" approach is sustainable, or whether it maybe makes sense to extend the |
| ("NEXT_LINE", ["Down", "Ctrl n"]), | ||
| ("SEND_MESSAGE", ["Ctrl d", "Meta Enter"]), |
There was a problem hiding this comment.
What's the motivation behind this change?
There was a problem hiding this comment.
I'd have the same question :) (just reposting this, because it's now my turn to review)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Could we adjust this approach to include the line number at which it was found along with the file name?
|
I manually tested this and reviewed the code - added 2 comments. Maybe making the commit summaries a bit more descriptive, but other than that, LGTM :) |
Tests updated to not use urwid_readline hotkeys, to avoid reliance on external libraries for hotkey commands.
Identify external commands using the added suffix. Search for both the suffix variable and the actual suffix.
Lint exclusions updated.
Niloth-p
force-pushed
the
category/1498-lint-and-sync-readline/pr
branch
from
bb6c9f3
to
03f36cc
Compare
Niloth-p
force-pushed
the
category/1498-lint-and-sync-readline/pr
branch
from
03f36cc
to
0495e29
Compare
There was a problem hiding this comment.
I've rebased this and updated the commit message of the 1st commit.
But, it appears the approach / direction to take would vary quite a lot from the current implementation. And as zormit pointed out, the priority in relation to other issues needs to be addressed before we proceed with discussing / implementing alternate approaches.
| ("NEXT_LINE", ["Down", "Ctrl n"]), | ||
| ("SEND_MESSAGE", ["Ctrl d", "Meta Enter"]), |
There was a problem hiding this comment.
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.
Niloth-p
added
the
further discussion required
What does this PR do, and why?
External discussion & connections
topicHow did you test this?
Self-review checklist for each commit
Visual changes
Our shortcuts missing in Readline's keymap:
On successful linting:
Linting errors: