Skip to content

Conversation

Icy-Thought
Copy link
Contributor

NOTE: I am still very new to elisp world and this is the second PR I write for an elisp pacakge. If there is something missing, please ping me and I'll make sure to fix it! :)

Solves: #85

@Icy-Thought
Copy link
Contributor Author

Icy-Thought commented Mar 6, 2025

One thing I am still puzzled by is the autoload of rustic-flycheck-setup. Should we change that, or retain it as is?

@CeleritasCelery
Copy link
Contributor

I reran the latest commit to make sure that was false failure.

https://github.com/emacs-rustic/rustic/actions/runs/13517402444/job/38352881470

It looks like markdown mode now depends on Emacs 28

@Icy-Thought
Copy link
Contributor Author

It's up to you, because upcoming PR's will have the same complaint. Maybe we should drop the tests for Emacs version > 28? A separate PR would be required for that though, since this is completely unrelated.

@CeleritasCelery CeleritasCelery merged commit 22a5ef8 into emacs-rustic:main Mar 10, 2025
3 checks passed
@CeleritasCelery
Copy link
Contributor

Thanks! Sorry this took so long.

@Icy-Thought
Copy link
Contributor Author

No worries, and glad to help out! :)

@jian-lin
Copy link

This PR causes compilation error.

Reproducing steps:

  1. cd $(mktemp -d)
  2. HOME=$PWD emacs -Q
  3. eval this
(progn
  (require 'package)
  (add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/") t)
  (package-install 'rustic))
  1. see the following error in *Compile-Log* buffer
Compiling file /tmp/tmp.C2ifkR6TQo/.emacs.d/elpa/rustic-20250310.1415/rustic-flycheck.el at Fri Mar 14 17:18:24 2025
rustic-flycheck.el:7:11: Error: Cannot open load file: No such file or directory, flycheck

Only non-flycheck users will see this error. This error is not great but harmless for them.

One way to avoid this error is to move the optional file rustic-flycheck.el to a separated MELPA package, as is recommended here. This is a good way to deal with optional dependencies in new MELPA packages. However, for existing MELPA packages, it can be seen as a breaking change because flycheck users will have to install an extra rustic-flycheck MELPA package.

@CeleritasCelery
Copy link
Contributor

Thanks for reporting this. Splitting into a separate package is going to be more of a headache than it's worth. I think we can solve this by removing this line and adding with-eval-after-load 'flycheck to this line.

@jian-lin
Copy link

Splitting into a separate package is going to be more of a headache than it's worth.

To be clear, splitting only changes the distribution of rustic and causes no change to the development and repo structure of rustic.

@jian-lin
Copy link

FWIW, (info "(elisp)Hooks for Loading") says

Normally, well-designed Lisp programs should not use ‘with-eval-after-load’.

@Icy-Thought
Copy link
Contributor Author

I should've tested in emacs -Q because that error was not triggered even after I removed the package locally. Very odd.
But here is a question, isn't the whole rustic-flycheck not supposed to be loaded in the first place if flycheck has not been loaded?

rustic/rustic.el

Lines 229 to 230 in 22a5ef8

(with-eval-after-load 'flycheck
(require 'rustic-flycheck)))

@jian-lin
Copy link

jian-lin commented Mar 14, 2025

All files get compiled during installation. That's why MELPA suggests distributing optional files/packages (rustic-flycheck.el here) separately.

@CeleritasCelery
Copy link
Contributor

To be clear, splitting only changes the distribution of rustic and causes no change to the development and repo structure of rustic

It doesn't change anything for the developers, but it does mean that users now need to be aware of and import that package. Since I would guess 90% of users are using flycheck and it previously worked out of the box that would cause a big support headache. Not worth it for such a trivial change.

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.

3 participants