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 extension detection for Vale #4

Merged
merged 1 commit into from
Jul 3, 2022
Merged

Add extension detection for Vale #4

merged 1 commit into from
Jul 3, 2022

Conversation

johnhamelink
Copy link
Contributor

@johnhamelink johnhamelink commented Jul 2, 2022

Use either the file name extension, or the value of flymake-vale-file-ext.

Fixes #2.

image

TODO:

  • Create a buffer-local variable that can be set by the user and will be checked before we try to determine the extension from the buffer-file-name.

Copy link
Owner

@tpeacock19 tpeacock19 left a comment

Choose a reason for hiding this comment

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

Just a comments. Also, could you update the README to reflect these changes? A section on flymake-vale-extension and how the user can modify it.

flymake-vale.el Outdated Show resolved Hide resolved
flymake-vale.el Outdated Show resolved Hide resolved
flymake-vale.el Outdated Show resolved Hide resolved
@johnhamelink
Copy link
Contributor Author

@tpeacock19 thanks! I've had a go at all outstanding feedback, let me know what you think :)

@tpeacock19
Copy link
Owner

This looks great! If you don't mind squashing the commits into one and I'll merge it asap.

Thanks a lot for your help.

Use a buffer-local variable for the extension if defined

- Fix unbalanced parenthesis
- Use a buffer-local variable for the extension if defined

Remove flymake-vale--guess-extension-from-mode

Remove `with-current-buffer` and `cond` from `flymake-vale--detect-extension`

Update the docstring for `flymake-vale--detect-extension`

Add documentation to show how to use `flymake-vale-file-ext`
@johnhamelink
Copy link
Contributor Author

@tpeacock19 there you go, force pushed! Very welcome, it's working great for me now, and I believe flymake-vale is the first to implement this feature! With org-mode support coming down the pipe, it makes Vale much more usable in my opinion.

@tpeacock19 tpeacock19 merged commit 224e93a into tpeacock19:main Jul 3, 2022
@tecosaur
Copy link

tecosaur commented Jul 7, 2022

A minor design question: has using a major mode -> extension alist been considered?

@tpeacock19
Copy link
Owner

There are two types of file extension markup scoping that Vale provides.

  1. Built-in Markup awareness:
    Markdown {md,mdown,markdown,markdn}
    AsciiDoc {adoc,asciidoc,asc}
    reStructuredText {rst,rest}
    HTML {htm,html,shtml,xhtml}
    XML {xml}
    Org {org}
    Code - A number of built-in Languages that can be found here
  2. Custom file extension based scoping: This can be any file extension defined by the user.

The first is that when there are multiple file extensions for a given major-mode there is no guarantee that we will pass the preferred extension.

If this was restricted to just the modes corresponding with the built-in capabilities, then it may work. As long as we pass anything within the extension sets shown above. However, when we move into any other major modes the extensions we default to may have detrimental effects.

For example, auto-mode-alist for latex-mode defaults to {ltx,sty,cl[so],bbl}. A user can have custom rules applied to sty files and a separate set for bbl files. In my mind, there is no appropriate default. I imagine it would be best to allow for the user to supply their own file extension if need be.

That being said, If we did want to move forward with this, I do think an alist would be the best solution.

@tecosaur
Copy link

Thanks for that explanation. Would it be overcomplicating things if there were two extension customisation variables:

  • flymake-vale-file-ext (nil or extension string)
  • flymake-vale-mode-file-exts (alist of mode and extension string)

If flymake-vale-file-ext is non-nil, then that can be used. If nil, then flymake-vale-mode-file-exts can be checked for an entry, finally falling back on the actual file extension if given.

@tpeacock19
Copy link
Owner

I think the alist would be useful if people would like to have set defaults for a number of modes. However, I again do not think we'd have great success trying to prepopulate that list for a decent amount of modes. For example:

(defcustom flymake-vale-mode-file-exts
  '((org-mode . ".org")
    (python-mode . ".py")
    ;; which one {htm,html,shtml,xhtml} ?
    (html-mode . ?)
    ;; which one {ltx,sty,cl[so],bbl} ?
    (latex-mode . ?)))

@tecosaur
Copy link

Hmm, I think there's a bit more

(defcustom flymake-vale-mode-file-exts
  '(;; Prose
    (org-mode . ".org")
    (markdown-mode . ".md")
    (gfm-mode . ".md")
    (xml-mode . ".xml") ; or code?
    (rst-mode . ".rst")
    (adoc-mode . ".asciidoc")
    ;; Code
    (c-mode . ".c")
    (csharp-mode . ".cs")
    (css-mode . ".css")
    (scss-mode . ".scss")
    (go-mode . ".go")
    ; ...
    ))

@tpeacock19
Copy link
Owner

Yes, it was meant more as an example for those modes with multiple extensions. Like c-mode could also be .h or .i. But, perhaps it is just fine to make a choice and have them identified in the Readme.

Also, I think it may be better to use the actual file extension over the alist. Since that removes any potential clash of expectations.

@tecosaur
Copy link

The actual file extension is a good bet for obvious reasons 😛 but what about buffers without a corresponding file?

@tpeacock19
Copy link
Owner

I'm thinking flymake-vale-file-ext > file-name-extension > flymake-vale-mode-file-exts > nil.

@tecosaur
Copy link

That looks reasonable to me 👍

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.

Idea: Provide the file extension when running vale
3 participants