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

Normalize setting names/keys #577

Closed
ggrossetie opened this issue Jun 22, 2022 · 4 comments · Fixed by #587
Closed

Normalize setting names/keys #577

ggrossetie opened this issue Jun 22, 2022 · 4 comments · Fixed by #587
Labels
💬 discussion Disccussion on new features, projects, etc... ✨ enhancement

Comments

@ggrossetie
Copy link
Member

Currently, setting keys are inconsistent. We are using both camelCase and _ as a separator. Given that the key is used to construct a title, I think we should use a strict convention.

Official documentation

The properties in your configuration will be a dictionary of configuration properties.
In the settings UI, your configuration key will be used to namespace and construct a title. Though an extension can contain multiple categories of settings, each setting of the extension must still have its own unique key. Capital letters in your key are used to indicate word breaks. For example, if your key is gitMagic.blame.dateFormat, the generated title for the setting will look like this:

Blame: Date Format

Entries will be grouped according to the hierarchy established in your keys. So for example, these entries

gitMagic.blame.dateFormat
gitMagic.blame.format
gitMagic.blame.heatMap.enabled
gitMagic.blame.heatMap.location

will appear in a single group like this:

Blame: Date Format
Blame: Format
Blame › Heatmap: Enabled
Blame › Heatmap: Location

https://code.visualstudio.com/api/references/contribution-points#contributes.configuration

Present state

  • asciidoc.asciidoctorpdf_command
  • asciidoc.previewFrontMatter
  • asciidoc.preview.style
  • asciidoc.preview.attributes
  • asciidoc.preview.breaks
  • asciidoc.preview.linkify
  • asciidoc.preview.fontFamily
  • asciidoc.preview.fontSize
  • asciidoc.preview.lineHeight
  • asciidoc.preview.useEditorStyle
  • asciidoc.preview.refreshInterval
  • asciidoc.preview.scrollPreviewWithEditor
  • asciidoc.preview.scrollPreviewWithEditorSelection
  • asciidoc.preview.markEditorSelection
  • asciidoc.preview.scrollEditorWithPreview
  • asciidoc.preview.doubleClickToSwitchToEditor
  • asciidoc.preview.openAsciiDocLinks
  • asciidoc.trace
  • asciidoc.use_asciidoctorpdf
  • asciidoc.use_kroki
  • asciidoc.registerAsciidoctorExtensions
  • asciidoc.wkhtmltopdf_path
  • asciidoc.forceUnixStyleSeparator
  • asciidoc.enableErrorDiagnostics
  • asciidoc.useWorkspaceRoot
Settings in VS code

Proposal

Removed
  • asciidoc.previewFrontMatter -> I think we should remove this setting since it's possible to configure skip-front-matter as an attribute: https://docs.asciidoctor.org/asciidoctor/latest/html-backend/skip-front-matter/
  • asciidoc.preview.scrollPreviewWithEditorSelection -> Replaced by asciidoc.preview.scrollPreviewWithEditor (was deprecated)
  • asciidoc.forceUnixStyleSeparator -> We are not using it??
Renamed
  • asciidoc.asciidoctorpdf_command -> asciidoc.pdf.asciidoctorPdfCommand: Asciidoc > Pdf > Asciidoctor Pdf Command
  • asciidoc.wkhtmltopdf_path -> asciidoc.pdf.wkhtmltopdfCommand: Asciidoc > Pdf > Wkhtmltopdf Command
    • Note: to be consistent with asciidoc.pdf.asciidoctorPdfCommand, we should support command line arguments, for instance: /path/to/wkhtmltopdf --enable-local-file-access
  • asciidoc.use_asciidoctorpdf -> asciidoc.pdf.engine: Asciidoc > Pdf > Engine
    • Either: wkhtmltopdf or asciidoctor-pdf (it also makes room for asciidoctor-web-pdf if we want to enable experimental support)
  • asciidoc.preview.attributes -> asciidoc.preview.asciidoctorAttributes: Asciidoc > Preview > Asciidoctor Attributes
  • asciidoc.preview.openLinksToAsciidocFiles: Asciidoc > Preview > Open Links To Asciidoc Files
    • Note: Asciidoc is not properly capitalized but it would be even worth to use AsciiDoc since it will produce "Ascii Doc"
  • asciidoc.use_kroki -> asciidoc.extensions.enableKroki: Asciidoc > Extensions > Enable Kroki
  • asciidoc.registerAsciidoctorExtensions -> asciidoc.extensions.registerWorkspaceExtensions: Asciidoc > Extensions > Register Workspace Extensions
  • asciidoc.useWorkspaceRoot -> asciidoc.useWorkspaceRootAsBaseDirectory: Asciidoc > Use Workspace Root As Base Directory
@ggrossetie ggrossetie added ✨ enhancement 💬 discussion Disccussion on new features, projects, etc... labels Jun 22, 2022
@ggrossetie
Copy link
Member Author

@danyill do you think we should do it as part of the next major version 3.0.0?

ggrossetie added a commit to ggrossetie/asciidoctor-vscode that referenced this issue Jun 26, 2022
- use third-person singular verb form
- introduce 5 categories : preview, pdf, extensions, general, debug
- use order to move most commonly used settings on top (of each category)
- deprecate/rename settings to comply with the new naming convention
- remove unused settings
- update english localization (need to update the japanese localization)
@danyill
Copy link
Contributor

danyill commented Jun 26, 2022

I like this proposal. I think the groupings are much clearer.

@danyill do you think we should do it as part of the next major version 3.0.0?

I guess a new major version is the right time to make a compatibility change.

@ggrossetie
Copy link
Member Author

I'm working on asciidoc.pdf.wkhtmltopdfCommand but I can't find a simple way to implement it. I think it would be better to have two settings, one for the binary path and one for the command line arguments.
Otherwise, it quickly gets complicated if the path contains spaces:

./dir\ with\ spaces/wkhtmltopdf --arg "foo bar"
"./dir with spaces/wkhtmltopdf" --arg "foo bar"

Arguably, we could rely on the PATH but I guess some users cannot modify the PATH on their corporate machine/laptop.
I also think we should simplify the default value to be either wkhtmltopdf (Linux, macOS) or wkhtmltopdf.exe.
And as mentioned in another thread, we should remove the download feature and instead ask users to download the latest version from https://wkhtmltopdf.org/downloads.html.

@ggrossetie
Copy link
Member Author

So:

  • asciidoc.pdf.wkhtmltopdfCommandPath
  • asciidoc.pdf.wkhtmltopdfCommandArgs
  • asciidoc.pdf.asciidoctorPdfCommandPath
  • asciidoc.pdf.asciidoctorPdfCommandArgs

ggrossetie added a commit to ggrossetie/asciidoctor-vscode that referenced this issue Jun 26, 2022
- use third-person singular verb form
- introduce 5 categories : preview, pdf, extensions, general, debug
- use order to move most commonly used settings on top (of each category)
- deprecate/rename settings to comply with the new naming convention
- remove unused settings
- update english localization (need to update the japanese localization)
ggrossetie added a commit that referenced this issue Jul 1, 2022
- use third-person singular verb form
- introduce 5 categories : preview, pdf, extensions, general, debug
- use order to move most commonly used settings on top (of each category)
- deprecate/rename settings to comply with the new naming convention
- remove unused settings
- update English localization
- update Japanese localization, thanks @YoshihideShirai 
- use markdownDeprecationMessage and markdownDescription to format deprecation message and description (using Markdown)
- add scope: resource (taken form the Markdown extension)
- update the logic around the wkhtmltopdf command (we need to support command line arguments)
- update the logic to select a PDF engine
- try to use asciidoctor-pdf first and then bundle exec asciidoctor-pdf if the asciidoctor-pdf command has not been defined
- offer to install asciidoctor-pdf in the global storage if the asciidoctor-pdf command has not been defined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 discussion Disccussion on new features, projects, etc... ✨ enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants