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

Add verible_formatter_verilog package #8950

Merged
merged 5 commits into from
Aug 14, 2024
Merged

Add verible_formatter_verilog package #8950

merged 5 commits into from
Aug 14, 2024

Conversation

sxr1223
Copy link
Contributor

@sxr1223 sxr1223 commented Jul 11, 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 unique, and 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: ERROR

Repo link: verible_formatter_verilog
Results help

Packages added:
  - verible_formatter_verilog

Processing package "verible_formatter_verilog"
  - ERROR: Branch-based releases are not supported for new packages; please use "tags": true
  - ERROR: The binding ['f11'] unconditionally overrides a default binding
    - File: Default.sublime-keymap

@braver
Copy link
Collaborator

braver commented Jul 16, 2024

Please reread that bit about keybindings in the PR template

You might not want to have a sublime-project file in your package. If you do want to check it in, exclude it from the final package by using a gitattributes file.

Your package is now called "verible_formatter_verilog" so that should also be the filename for the settings file. You can use capitals and spaces to create a more readable package name though. Snake_case is a bit unusual here.

Since you're just wrapping a formatter executable, I wonder if Fmt might also be able to handle that. At the very least your readme should explain how to install any programs your package relies on.

You try to expose all formatting options via your package settings file. Wouldn't it be more flexible for endusers to use the standard settings file used by these tools, so that the same settings are also used outside of Sublime Text?

@kaste
Copy link
Contributor

kaste commented Jul 16, 2024

Can you translate the status messages to English? I think "Verible Formatter" is a nice name btw.

@sxr1223
Copy link
Contributor Author

sxr1223 commented Jul 17, 2024

Thank you for your time. Following your advice, I have removed unnecessary files and default key bindings, and renamed plugin to VeribleFormatterVerilog. I prefer configuring everything in Sublime, so I have added all formatting options to the package settings. However, I agree with your suggestion to incorporate this feature into the package settings using a flag file or Sublime configuration.

Fmt is an excellent plugin. When searching for Sublime plugins under "Verilog," I initially overlooked this plugin. However, due to Verible's limitations, when there are syntax errors in the Verilog code, Verible fails to format it, and fmt cannot display error messages, which complicates corrections. Additionally, Verible cannot parse `include, thus it cannot format files that include `include. My plugin is designed to address these shortcomings.

As Chinese is my native language, I prioritize using it. I will endeavor to add English support later.

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: ERROR

Repo link: VeribleFormatterVerilog
Results help

Packages added:
  - VeribleFormatterVerilog

Processing package "VeribleFormatterVerilog"
  - ERROR: Branch-based releases are not supported for new packages; please use "tags": true

@braver
Copy link
Collaborator

braver commented Aug 7, 2024

  • you've got a few print statements in there (e.g. print(flags_file_path))
  • you can simply a bit by removing the defaults whenever you do settings.get() since your settings file already has defaults
  • you currently only support a global flags file using an absolute path. usually it's convenient to have such a file in the root directory of the project (so that each project can have its own settings)

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: VeribleFormatterVerilog

Packages added:
  - VeribleFormatterVerilog

Processing package "VeribleFormatterVerilog"
  - All checks passed

@sxr1223
Copy link
Contributor Author

sxr1223 commented Aug 9, 2024

Thanks for your advise. I have modified my plugin according to your advise and added some feature.

@braver
Copy link
Collaborator

braver commented Aug 14, 2024

Final comments, you're welcome to have a look at these at some later time:

// specify flag file name under project. if exit, verible will use the flag file under project, 
// not `flags_file_path` or setting specified below.

for clarity perhaps change to

// specify flag file name as used in your current project.
// if it exists, verible will use this instead of the "global_flags_file_path".

You expect the global_flags_file_path to be place inside the user's Sublime Text packages directory. That's not usually a good place for file like that. It's intended for plugins and settings for ST itself, whereas this is a config file for the command line utility you're making available. At the very least it should be in the User package directory. But more commonly you would put these in the user's home directory (e.g. ~/ on Unix-like systems). Look here for some guidance on getting that path using Python in a cross-platform compatible way: https://stackoverflow.com/questions/2668909/how-to-find-the-real-user-home-directory-using-python#2668952

@braver braver merged commit a0b6af5 into wbond:master Aug 14, 2024
2 checks passed
@sxr1223
Copy link
Contributor Author

sxr1223 commented Aug 25, 2024

Thank you for your advice. I’ve updated the comments based on your suggestions. However, I must stick with the current location of the configuration file because it ensures an out-of-the-box experience—users can start using the plugin right after installation without needing to modify it according to the README. Users who prefer a different configuration can still manually change the location of the configuration file. Therefore, I believe the current setup is more suitable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants