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

Added Token Generator Plugin #8910

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Added Token Generator Plugin #8910

merged 1 commit into from
Sep 3, 2024

Conversation

pxninja
Copy link
Contributor

@pxninja pxninja commented Apr 15, 2024

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use .gitattributes to exclude files from the package: images, test files, sublime-project/workspace.

My package is a random token generator intended to be used with CSS, but could be used in other contexts.

There are no packages like it in Package Control.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: Token Generator

Packages added:
  - Token Generator

Processing package "Token Generator"
  - All checks passed

@braver
Copy link
Collaborator

braver commented Jun 19, 2024

I guess I can see how this could be useful, although doubtful I'd approve a PR with lots of this stuff in the CSS 😅 Anyway, two points of feedback:

  • Please reconsider shipping the keybinding (as per the PR template)
  • Please provide a way to disable the context menu entry

@pxninja
Copy link
Contributor Author

pxninja commented Jul 5, 2024

Thank you for the feedback.

  • The context menu entry can now be disabled by changing "show_in_context_menu" to "false" in the settings.
  • If you're aware of a way to do something similar for keybindings (enabling / disabling via settings) happy to include that as well. Otherwise I'm inclined to error on the side of accessibility and convenience.

@kaste
Copy link
Contributor

kaste commented Jul 9, 2024

Hi!

I would not provide a key-binding, t.i. for this type of command I would recommend opt-in instead of opt-out.
The way to go would be to remove all *.sublime-keymap files and then to add one Default.sublime-keymap
with the contents commented out (// ).

Then in the commands add an entry, e.g. https://github.com/sublimelsp/LSP/blob/293f4a4340cca5ab1ad065643e4f20d9b270b2b1/Default.sublime-commands#L10-L18

    {
        "caption": "Preferences: Token Generator Key Bindings",
        "command": "edit_settings",
        "args": {
            "base_file": "${packages}/Token Generator/Default.sublime-keymap",
            "user_file": "${packages}/User/Default ($platform).sublime-keymap",
            "default": "[\n\t$0\n]\n",
        }
    },

Note that the provided bindings on Linux and Windows ("ctrl+shift+option+t") are invalid as option
is unused on those platforms. (That's a living example of why a key binding here is often over-protecting; usually we don't know the other platforms that well.)

Of course, they are ways to program an opt-out for key-bindings but these either add computational overhead on the user side or maintenance burden on the dev side. I don't think this would be justified in this case.

You currently use new_token as the command name, however, these names are a shared resource thus you must namespace your commands, e.g. use token_generator_new_token.
Only Sublime Text core commands don't have a prefix.

Lastly, there is a mismatch how this plugin/package is named. You use both "Token Generator" and token-generator. E.g. in this PR

            "name": "Token Generator",
            "details": "https://github.com/pxninja/token-generator",

Note that the name Token Generator is the final name. (In a way you rename your repo from token-generator to Token Generator basically like a git clone https://github.com/pxninja/token-generator 'Token Generator'.) Hence you should use "Token Generator" for the file names. (It is also not too late to rename your repo.)

E.g., in your commands file, https://github.com/pxninja/token-generator/blob/43a12dc35d47bd6984ca952aadc3f5ab7713fb0e/Default.sublime-commands#L6-L14

  {
    "caption": "Token Generator: Settings",
    "command": "edit_settings",
    "args":
      {
        "base_file": "${packages}/token-generator/token-generator.sublime-settings",
        "default": "{\n\t$0\n}\n",
      }
  }

but the base_file will be actually ${packages}/Token Generator/token-generator.sublime-settings. And the settings file itself should match that and be like Token Generator.sublime-settings.

People who don't like spaces in paths may use "TokenGenerator", or just stick with token-generator. Refer as an example https://github.com/Azd325/sublime-text-caniuse/blob/7da53dd3e0a999b438d582e7971c35c207af388c/Main.sublime-menu#L32-L34, where the repo name is sublime-text-caniuse but the plugin is named "Can I Use".

Happy coding.

@FichteFoll
Copy link
Collaborator

Note that the provided bindings on Linux and Windows ("ctrl+shift+option+t") are invalid as option is unused on those platforms.

FWIW, option is actually aliased to alt on Linux and Windows.

@pxninja
Copy link
Contributor Author

pxninja commented Jul 11, 2024

  • Keybinding is now opt-in
  • token-generator.sublime-settings is now Token Generator.sublime-settings
  • token-generator.py is now Token Generator.py
  • token-generator within paths is now Token Generator

This feedback is not clear to me:

You currently use new_token as the command name, however, these names are a shared resource thus you must namespace your commands, e.g. use token_generator_new_token.

Are you saying the primary class needs to be renamed to TokenGeneratorNewTokenCommand, and then update each reference to token_generator_new_token?

@kaste
Copy link
Contributor

kaste commented Jul 11, 2024

Are you saying the primary class needs to be renamed to TokenGeneratorNewTokenCommand, and then update each reference to token_generator_new_token?

Yes, that's what we all usually do (except in the early wild-west days of Sublime). Or maybe tg_new_token if that's too long. Pro-tip but some hate the look: name the class token_generator_new_token then Sublime's "Goto definition" works. E.g. in the key-bindings place the cursor on token_generator_new_token and then "Goto definition" will jump to the python file.

Spaces in python files (Token Generator.py) itself are strange -- didn't expect that to work actually -- beware that you cant import them. The typical name would be (🙄) token_generator.py or a generic plugin.py/main.py as neither import token-generator nor import Token Generator should work. (But if it works, who to judge.)

@pxninja
Copy link
Contributor Author

pxninja commented Jul 11, 2024

  • Command name has been updated.
  • File names have received 2 sets of feedback and I'm not sure how to reconcile.

Both this ...

you should use "Token Generator" for the file names

... and this ...

The typical name would be token_generator.py

I'm okay with whatever naming convention is preferred. I'm simply confused as to what the requirements are.

@kaste
Copy link
Contributor

kaste commented Jul 11, 2024

Yeah that was a bit short. Take the package name -- as you did -- for the special Sublime files, e.g. "Token Generator.sublime-settings". But the python file should be a valid python module name, and then neither spaces or dashes are allowed. So you go with token_generator.py or any generic name like main.py/entrypoint.py/plugin.py.

@braver
Copy link
Collaborator

braver commented Jul 16, 2024

yeah, what he said :)

@pxninja let me know when you've handled that, are happy with anything else, and tagged a new release. We're almost ready to hit merge here 🙂

@pxninja
Copy link
Contributor Author

pxninja commented Jul 16, 2024

  • The main file has been renamed to token_generator.py
  • A new release has been tagged v1.0.1

@braver
Copy link
Collaborator

braver commented Aug 7, 2024

I see that you've added a setting to disable the context menu item. Much appreciated. However, the is_visible method is going to be used for all menus, so that setting will probably also make the command disappear from the command palette. The way I tend to handle that is by passing a "context" argument, and then checking that argument.
Example: https://github.com/braver/SideBarTools/blob/master/Tab%20Context.sublime-menu#L13
And handling it: https://github.com/braver/SideBarTools/blob/master/SideBar.py#L30

@braver braver added the mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side label Aug 7, 2024
@pxninja
Copy link
Contributor Author

pxninja commented Sep 2, 2024

  • Added args to Context.sublime-menu
  • Added if statement to is_visible in token_generator.py
  • Tagged new release v.1.0.2

@braver braver merged commit a69bf31 into wbond:master Sep 3, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided mergeable The PR is in a mergeable state but awaiting some final comments/acknowledgement from either side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants