hotkeys: Add linting for hotkeys.

[srdeotarse]

Fixes #1209

Adds GitHub Actions and changes to lint-all for linting generate_hotkeys.py

What does this PR do?

Tested?

• [x] Manually
• [ ] Existing tests (adapted, if necessary)
• [ ] New tests added (for any new behavior)
• [x] Passed linting & tests (each commit)

Commit flow

Notes & Questions

Interactions

Visual changes

[neiljp]

@srdeotarse This contains quite a few different types of changes in one commit; it would be more useful to split each change into its own commit - they can always be squashed together later, and often reordered too, if independent.

Please check out my comment in #1209, but this PR currently contains elements to:

• make fix one additional tools/ file itself; this doesn't seem so useful, and we might consider multiple files (eg. more of tools/ in general). I suspect this was confusion over my thought of running the script in make fix and my third point in #1209
• parse the help text and lint it for errors; this was my second comment in #1209
• run mypy on one file (fits with the first bullet here again)

This doesn't actually test whether the hotkeys are in sync in CI yet (first point in #1209).

I hope this makes it clearer! Please discuss in the stream further if still unclear :)

[zulipbot]

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

[Rohitth007]

@srdeotarse It would be easier to review your PRs, if you start resolving any of the open comments there might be if they are resolved, unless you are waiting for a response or want a second opinion. It can get confusing to see many open comments, especially if only partly addressed. (Not specific to this PR)

Apart from that, I wanted to ask if we want to do duplicate checking per category or over all keys. Currently it seems to be for per category.

[srdeotarse]

@Rohitth007 I have received no confirmation from @neiljp about duplicate checking per category or over all keys. I have suggested for per category because the same shortcut may work based on where the user is currently in ZT (for e.g. esc to exit from compose area, stream messages or stream search). I know there is a common function for esc but, just using it to explain my point.

Thank you for reviewing my code and the point noted about resolving the conversation.

[Rohitth007]

@srdeotarse I've left a few comments, mostly related to diffs being more readable. This makes reviewing much easier.

Apart from that, please acknowledge the reviews with either a :+1: or saying some version of "I've fixed this" or answering the questions before resolving them or whenever you push a new update, so that the reviewer knows what's addressed and what is not. It even makes it easier for a new person going through the conversation.

[neiljp]

@srdeotarse Thanks for all your work on this - this was close enough that I worked through a few minor issues to get it merged, which I'll do after pushing to here :tada:

Functionally, the only issue seemed to be not failing on being out of sync, and the GH job being out of date and based on pytest rather than simpler linting. Commit-wise, a little code was also in the wrong commit.

Other than fixing those functional issues I made some commit ordering changes, partly to explain the commit organization in earlier comments; these weren't strictly necessary, but I hope you can see how the flow is a little clearer.

An overview of the changes per-commit:

• first two commits as-is except slight commit text change
• 3rd commit combined with other function extraction commit (6th) and a docstring reword, also retaining order of code from before to minimize diff
• 4th commit (reorder) now smaller due to reordering in 3rd commit [- 5th commit adding extra function dropped (as per discussion)] [- 6th commit integrated into 3rd (as above)]
• 7th commit slightly reordered and reworded (it adds keys/output variables)
• added extra refactor to refer to those variables in docstrings (later commits adjusted too)
• 8th commit minor commit text change, add extra error_flag=1 if files don't match
• 9th commit as-is, plus code from 10th commit that was specific to duplication linting
• 10th commit tool slightly renamed, extra code split into previous commit (duplication related)
• 11th commit moved job below other linting jobs & adapted for recent changes (+lint rename)

I'd suggest you verify the changes by using tools/fetch-pull-request 1210 and doing a git range-diff main hotkey-linting review-original-1210, though the output format can be confusing at times. That may help you understand changes I've made in detail.

Returning to the issue of the order of the linting commits, in principle it might have been better if the 8th commit with the linting function had been split into just linting for differences (ie. needs sync or not), and the description linting, since the former could then be combined with the changes to main() to make a minimal working linting option. The linting for description and duplication could then be extensions built on top in later commits. The linting code would then be testable from the start. However, given the commits seem functionally fine, I didn't want either of us to spend more time on that - just something to bear in mind for the future :)