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 defcustom for syntax checker to use. #29

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tmcgilchrist
Copy link

Defaults to existing flymake but can be overridden with flycheck. For example using use-package or regular M-x customize. As suggested in #28

(use-package ocaml-eglot
  :custom
  (setq ocaml-eglot-syntax-checker 'flycheck))

This will override syntax checking so it doesn't try using flymake.

Defaults to existing flymake but can be overridden with flycheck.
For example using use-package or regular customize.

(use-package ocaml-eglot
  :custom
  (setq ocaml-eglot-syntax-checker 'flycheck))
@tmcgilchrist tmcgilchrist force-pushed the customize-syntax-check branch from c8ab4c7 to 4f48cfd Compare February 23, 2025 23:34
@@ -33,6 +33,7 @@
;;; Code:

(require 'flymake)
(require 'flycheck)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a good idea to require flycheck here, as it's not installed by default. If someone's using flycheck it will likely be already loaded by the time ocaml-eglot gets loaded, so you can just declare whatever functions from it are going to be used here to silence the byte-compiler.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not (yet!) really familiar with scope presence and require.
What's the best way to avoid relying on flycheck by default? Is it possible, at the ocaml-eglot level to behave differently if flycheck is installed and ideally to rely on additional behavior in case of flycheck-eglot?

@bbatsov
Copy link
Contributor

bbatsov commented Feb 24, 2025

I'm guessing if someone's using Flycheck with Eglot it's a good idea to mention https://github.com/flycheck/flycheck-eglot, so it replaces Flymake everywhere.

Makefile Outdated
@@ -1,6 +1,6 @@
# Space-separated list of the dependencies of your project (include
# package-lint and/or buttercup if you want makel to use these tools):
ELPA_DEPENDENCIES=package-lint
ELPA_DEPENDENCIES=package-lint,flycheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple items should be separated by spaces, not commas. (that's why the CI is failing)

That being said - I don't think it's a good idea to add flycheck here.

Also it's the maintainers might want to check out some Emacs-specific build tool like Eldev that can simplify things like linting, running tests, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I used Makel because @DamienCassou very kindly answered my naive questions about Emacs :) However, I am open to any alternative (or complementary proposal)!

Choose a reason for hiding this comment

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

Emacs-specific build tool like Eldev that can simplify things like linting, running tests, etc.

@bbatsov: makel is definitely Emacs-specific and can simplify things like linting and running tests. I'm not saying it's the only or best option though. It's just mine and I use it in all my projects. makel's README suggests alternative options if you don't like it.

@tmcgilchrist
Copy link
Author

@bbatsov thanks for the comments. I basically did the minimal thing here, but it would be cleaner to conditionally depend on flycheck (if that is possible) or even better not define keybindings and show how to set them up using use-package.

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