Integrating Editorconfig support into Emacs

[monnier]

Hi @10sr,

I sent you email discussing a possible plan for integration into Emacs (mostly focusing on replacing the two hackish advice with hooks that give us the "right" access). I have pushed the corresponding WiP patches to the
scratch/editorconfig branches of both the emacs.git and the nongnu.git repositories (different patches: the ones in emacs.git are the patches which add the hooks that let us specify additional dir-local variables and to specify the coding system to use, whereas the patches in nongnu.git are corresponding changes in the code of editorconfig-emacs to make use of those new hooks).

If we want to include some support in Emacs-30, we need to move quickly (Eli is about to cut the branch for Emacs-30).
The main concern is the fact that the hack-dir-local-variables machinery manipulates lists of var settings before applying them, so we can't reuse the existing code as-is and I'd be interested in your opinion about how to go about it.

[10sr]

🙆
I'll check this weekend.

[monnier]

🙆 I'll check this weekend.

Thanks!

[10sr]

I read patches you cc-ed to me.
It sounds really great that a new hook is going to be provided and editorconfig can make use of it!

It seems we can still use your editorconfig--get-dir-local-variables function for Emacs<30 if we (mainly I, maybe!) update the existing code to use it (please tell me if it is not true), so I'm totally OK with it 👌

Some comments from editorconfig.patch:

FIXME: Cache the result of `editorconfig-call-get-properties-function'?

FYI, editorconfig-core-handle.el already has a caching mechanism. This impl is used by default so normally .editorconfig file will not be parsed every time a file is opened.

;; FIXME: I don't understand the above comment, because
;; all major modes are supposed to call
;; `kill-all-local-variables' directory or indirectly.

rpm-spec-mode is not defined using define-derived-mode and calls kill-all-local-variables explicitly, which reset all editorconfig settings. So it is a matter of timing when it is called, not whether it is called or not...
At least when I wrote this code it is required, but I updated the impl afterword. So it is possible we can remove this now (I'll check later).

---

I also want to ask one thing to you: how the development process of editorconfig-emacs will change (or not change) after it is integrated into emacs?
As you can see editorconfig-emacs is currently developed mainly on GitHub, and also it often accept PR from users (especially adding new major-mode)
I'm not very familiar with the development of Emacs, is it possible to continue developing this plugin on GitHub?

[monnier]

It seems we can still use your `editorconfig--get-dir-local-variables` function for Emacs<30 if we (mainly I, maybe!) update the existing code to use it (please tell me if it is not true), so I'm totally OK with it! 👌

Indeed, I think we can keep the two code paths unified to a large extent. I'm working on such a patch.

> FIXME: Cache the result of `editorconfig-call-get-properties-function'? FYI, editorconfig-core-handle.el already has a caching mechanism(editorconfig-core-handle.el#L85-L101). This impl is used by default so normally .editorconfig file will not be parsed every time a file is opened.

Right. My question was whether we should try and avoid the cache lookup: the "old" code looks up the cache once per file-visit, whereas now we call it once for `hack-dir-local-get-variables-functions` and once for `auto-coding-functions`. But indeed, I'm going with "no" as the answer until proven otherwise. 🙂

> ;; FIXME: I don't understand the above comment, because > ;; *all* major modes are supposed to call > ;; `kill-all-local-variables' directory or indirectly. [rpm-spec-mode](https://github.com/stigbjorlykke/rpm-spec-mode/blob/c1c38050c48ea330c7cea632b8785d66daeefb2b/rpm-spec-mode.el) is not defined using define-derived-mode and calls kill-all-local-variables explicitly, which reset all editorconfig settings. So it is a matter of timing when it is called, not whether it is called or not... At least when I wrote this code it is required, but I updated the impl afterword. So it is possible we can remove this now (I'll check later).

I see, thanks. In any case this only affects the code for those Emacsen without `hack-dir-local-get-variables-functions`.

I also want to ask one thing to you: how the development process of editorconfig-emacs will change (or not change) after it is integrated into emacs? As you can see editorconfig-emacs is currently developed mainly on GitHub, and also it often accept PR from users (especially [adding new major-mode](https://github.com/editorconfig/editorconfig-emacs/pulls?q=is%3Apr+is%3Aclosed+-mode)) I'm not very familiar with the development of Emacs, is it possible to continue developing this plugin on GitHub?

That was my question as well: Also, I'm curious to hear what you'd like to do with that package in the longer run: would you be interested to move the development and maintenance to the code that will be in `emacs.git` (and basically treat `editorconfig-emacs` as legacy code maintained only for older Emacsen), or maintain both in parallel (in which case we'll want to be careful to keep the two codes in sync and to make sure such sync is easy enough to do), or would rather just keep working on `editorconfig-emacs` and let other people deal with the copy of the code we'll install into `emacs.git`, ... ? So, IIUC you prefer to keep developing it in `editorconfig-emacs`? Stefan

[10sr]

Right. My question was whether we should try and avoid the cache lookup:
the "old" code looks up the cache once per file-visit, whereas now we
call it once for hack-dir-local-get-variables-functions and once for
auto-coding-functions.

But indeed, I'm going with "no" as the answer until proven otherwise. 🙂

I see 🙆

That was my question as well:

Also, I'm curious to hear what you'd like to do with that package in the
longer run: would you be interested to move the development and
maintenance to the code that will be in `emacs.git` (and basically treat
`editorconfig-emacs` as legacy code maintained only for older Emacsen),
or maintain both in parallel (in which case we'll want to be careful to
keep the two codes in sync and to make sure such sync is easy enough to
do), or would rather just keep working on `editorconfig-emacs` and let
other people deal with the copy of the code we'll install into
`emacs.git`, ... ?

So, IIUC you prefer to keep developing it in editorconfig-emacs?

Thanks!
Yes, and the second idea (maintain both in parallel (in which case we'll want to be careful to
keep the two codes in sync and to make sure such sync is easy enough to
do)) sounds preferable to me, if possible.

[monnier]

I pushed to scratch/editorconfig (in nongnu.git) a new set of patches which makes it use the new
hooks when available.
[ Sadly, Github doesn't let us submit pull requests from other repositories. Hopefully ForgeFed will address such issues in the future. ]

To consult that branch you can do:

git remote add -ft scratch/editorconfig nongnu git://git.sv.gnu.org/emacs/nongnu.git
git log nongnu/scratch/editorconfig

I have not had much luck running the tests and haven't been able to figure out yet how they work, tho, so there's probably some breakage.

[monnier]

Regarding the tests: when I try to run them on the current version of the package, I get:

3 unexpected results:
   FAILED  test-charset
   FAILED  test-editorconfig
   FAILED  test-trim-trailing-ws

when running them using Emacs-29 on my new code (but still using the old hooks/advices) I get the same 3 failures plus:

FAILED  test-lisp-use-default-indent

and on Emacs-30 (i.e. when using the new hooks) I get two more test failures:

6 unexpected results:
   FAILED  test-charset
   FAILED  test-editorconfig
   FAILED  test-hack-properties-functions
   FAILED  test-lisp-use-default-indent
   FAILED  test-local-variables
   FAILED  test-trim-trailing-ws

I don't know how to run the tests' code by hand (e.g. I can't find the trim.txt file used during the test-trim-trailing-ws nor the .editorconfig presumably placed nearby), so I have not been able to investigate the origin of any of those tests failures.

[ BTW, if you could comment on issue #343, that would be helpful. ]

[10sr]

Thanks!
Sorry I haven't had much time for this yet, I'll check your branch later.

I don't know how to run the tests' code by hand (e.g. I can't find the trim.txt file used during the test-trim-trailing-ws nor the .editorconfig presumably placed nearby), so I have not been able to investigate the origin of any of those tests failures.

plugin-tests/test_files/ is a git submodule directory so you'll need git submodule update --init first.

[monnier]

`plugin-tests/test_files/` is a git submodule directory so you'll need `git submodule update --init` first.

Aha!

[monnier]

OK, I made progress: now the new code passes all the tests when run in Emacs<30 and there are only 2 new test failures when running in Emacs-30:

• In test-charset we apparently check that the buffer-file-coding-system is also set when opening a new file, but with the new hook, this is not the case: instead auto-coding-functions will be reconsulted when we write the file. So the test fails, but it should not be a problem in practice.
• In test-local-variables we fail completely because the new hook is not really compatible with editorconfig-override-file/dir-local-variables.

[KaratasFurkan]

It seems it's merged, congrats and thanks @monnier @10sr !

— a happy Emacs user who thinks editorconfig should be supported by default

[10sr]

Almost all the work for this integration was done by @monnier, and I just answered some questions from them on GitHub.
Really appreciate it, thanks!