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

Lint the validity of help categories #1518

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Niloth-p
Copy link
Collaborator

What does this PR do, and why?

Check for typos in category names.
Previously, these typos would go unnoticed, with the key binding going missing from the help menu.

Outstanding aspect(s)

  • Feedback welcome on the formatting and phrasing of the error message printed
  • This could be put on hold for the moment and be merged along with the major changes we will be doing to lint-hotkeys after the addition of contexts. But, that might take a while.

External discussion & connections

  • Discussed in #zulip-terminal in topic
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

image

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Jun 15, 2024
Copy link
Member

@rsashank rsashank left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @Niloth-p!
The intention is clear. I've tested this on my PC and reviewed the code, and it LGTM 👍.

Copy link

@zormit zormit left a comment

Choose a reason for hiding this comment

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

That seems like a nice idea to check for this and will hopefully improve the developer experience :) Thanks for working on this!

It's a bit adjacent to this PR, but while you're in this area of the code, I've left a comment to improve it. I think it wouldn't hurt to fix it as part of this PR, as it's a very small PR anyways.

I'm still approving it, because in my opinion it's optional to fix.

@@ -39,8 +39,8 @@ def lint_hotkeys_file() -> None:
existing OUTPUT_FILE
"""
hotkeys_file_string = get_hotkeys_file_string()
error_flag = 0 if is_every_key_category_valid() else 1
Copy link

Choose a reason for hiding this comment

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

it's rather independent of this change, but it's quite strange to me to use 0 and 1 instead of a boolean here. I know sys.exit wants to have an integer, but it should in my opinion only be converted to it at the very last moment.

@zormit zormit added PR needs review PR requires feedback to proceed and removed PR needs mentor review labels Jun 27, 2024
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Jun 28, 2024
tools/lint-hotkeys Outdated Show resolved Hide resolved
@Niloth-p Niloth-p force-pushed the 1518-lint-valid-categ/pr branch from b02cbe6 to 82dccf2 Compare June 28, 2024 08:04
@Niloth-p Niloth-p mentioned this pull request Jul 18, 2024
12 tasks
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Niloth-p As discussed briefly via video, this would have been useful when I rebased a PR already :)

I left a few small notes and otherwise looks good to go. As per @zormit's notes, the refactoring isn't vital but would make it cleaner, particularly as we don't have tests for this.

if key_category not in HELP_CATEGORIES:
print(
f"Invalid key_category '{key_category}' for key '{key}'."
f" Choose a category from:\n{', '.join(HELP_CATEGORIES.keys())}\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: For a dev tool this isn't as important, but with this number of categories we already run quite far to the right, so a vertical list may be cleaner to read.

Comment on lines +79 to 80
error_flag = True
sys.exit(error_flag)
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this likely works, an explicit conversion here would be good, as per #1518 (comment)

error_flag = 0 if is_every_key_category_valid() else 1
error_flag = not is_every_key_category_valid()
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: As it's now a boolean, it'd also be useful to have a cleaner name for this.

Maybe is_any_error_present, or similar.

@neiljp 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 Jul 22, 2024
@Niloth-p
Copy link
Collaborator Author

Niloth-p commented Jul 22, 2024

Thank you for the review, @neiljp.
Ah, I did not think about these details in the same manner. Thank you for your perspective.

I've actually absorbed these 2 commits into #1484's refactoring.

We could finish these here, and I'll have to rebase that branch to make the flow still make sense (I would prefer to avoid that if possible, as it could lead to making silly errors and code degradation in the worst case (like it happened once before with the hotkeys.md)).
Or we could make these changes there in #1484 to keep the sequencing, if we're not in a hurry to get this merged? (That should be quite straight forward).

See

@Niloth-p Niloth-p added PR replaced by another PR and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 22, 2024
@rsashank rsashank mentioned this pull request Sep 6, 2024
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR replaced by another PR size: M [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants