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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants