Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refact: Add lowercase conversion for keycodes #637

Closed

Conversation

jeevithakannan2
Copy link
Contributor

@jeevithakannan2 jeevithakannan2 commented Sep 23, 2024

Type of Change

  • Refactoring

Description

  • Convert the keys to lowercase eliminating the need to specify both the capital and non-capital keycodes for a specific action.
  • Keys that don't need to be converted to lowercase for example theme switching can be declared in the
    KEYS_NOT_TO_NORMALIZE
  • Renamed function toggle_task_list_guide to enable_action_guide for consistency

Testing

  • Works as expected no issues.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no errors/warnings/merge conflicts.

Copy link
Contributor

@cartercanedy cartercanedy left a comment

Choose a reason for hiding this comment

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

I don't like this, extra complication for dubious benefit, at best

@adamperkowski
Copy link
Collaborator

adamperkowski commented Oct 4, 2024

I agree with @cartercanedy. I think this (if at all) should be handled within the modules that use keycodes.

@ChrisTitusTech
Copy link
Owner

Yeah, I am ok with hotkeys being cap sensitive. We might need to specify different actions for a Capital letter and I don't want to normalize this.

@jeevithakannan2 jeevithakannan2 deleted the keys-refactor branch October 9, 2024 00:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants