Skip to content

Conversation

@xendk
Copy link
Contributor

@xendk xendk commented Aug 3, 2025

I implemented
flycheck-eldev--checker-substituted-arguments to handle the
'source-inplace to 'source conversion first. Then I ran the tests
and had lots of failures. Fixing up the --setup-first made all the
tests green again.

@doublep
Copy link
Member

doublep commented Aug 3, 2025

Please split into two PRs: one to fix things that got broken in the meanwhile (lexical-binding and so on), the other fully about new functionality.

@xendk xendk force-pushed the watcher-compatibility branch 2 times, most recently from 3e59793 to 1e7b81b Compare August 7, 2025 18:50
@xendk
Copy link
Contributor Author

xendk commented Aug 7, 2025

I've rebased the branch and after updating the copyright year all but snapshot passes.

xendk added a commit to xendk/dotemacs that referenced this pull request Aug 10, 2025
@doublep
Copy link
Member

doublep commented Aug 17, 2025

Looks fine, but still need some tests and a way to disable this.

About the failure on snapshot: this is caused by changes in Emacs: https://lists.gnu.org/archive/html/emacs-devel/2025-08/msg00332.html. I have managed to arrange some changes here, but they still haven't been committed (presumably Philip is on vacation). You may want to ping it after some time, would carry more weight if more people show up.

@xendk
Copy link
Contributor Author

xendk commented Aug 17, 2025

Looks fine, but still need some tests and a way to disable this.

Well, the existing tests cover this, and I'd like to advocate not making it disable-able.

For starters, we'd need to duplicate a lot of test between the two cases to ensure that they indeed do the same thing.

Secondly, the most invasive part of this change (the --setup-first change) builds upon existing code that eldev--package-dir-info doesn't find the original package file. If a (Emacs or other) change breaks that, there's a good chance that it would break the original too,

I would argue that this method is an improvement, in part because of a change I forgot somehow. With this PR, the following becomes unnecessary:

              ;; When checking project's main file, use the temporary as the main file
              ;; instead.
              "--setup"
              ,(flycheck-sexp-to-string
                `(when (and eldev-project-main-file (file-equal-p eldev-project-main-file ,real-filename))
                   (setf eldev-project-main-file ,filename)))

When we're not faking an empty file to eldev--package-dir-info for the original file in order to avoid clashes between it and the flycheck_ file, but instead feeding it the contents of the latter when it looks at the former, we don't need to fiddle with eldev-project-main-file, so this can be removed.

Lastly, in the case that flycheck-eldev--checker-substituted-arguments fails, or the user strong-arms it into using source-inplace anyway, the new version still works,

@doublep doublep force-pushed the watcher-compatibility branch from 7e44e75 to 3389a50 Compare August 18, 2025 19:34
@doublep
Copy link
Member

doublep commented Aug 18, 2025

Well, the existing tests cover this

I added more tests for multifile "project" and rebased your branch. Seems to work fine.

I will think about what else to do tomorrow.

@xendk xendk force-pushed the watcher-compatibility branch from a9c02c8 to f06e2b4 Compare September 9, 2025 21:55
@xendk xendk force-pushed the watcher-compatibility branch from f06e2b4 to abfbf12 Compare September 10, 2025 18:49
@doublep doublep merged commit e401a37 into flycheck:master Oct 25, 2025
11 of 12 checks passed
@doublep
Copy link
Member

doublep commented Oct 25, 2025

Sorry that it took so long.

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.

2 participants