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

Require :mode for all syntax checkers #1071

Merged
merged 2 commits into from Sep 12, 2016

Conversation

3 participants
@lunaryorn
Contributor

lunaryorn commented Sep 5, 2016

Follow up of #961: We've got no built-in syntax checkers without :mode anyway, thus making it mandatory simplifies our code.

Should we need a more generic system I think we should rather take the route of supporting derived modes. Even a hypothetical ispell checker could probably not support all modes: There are fundamental differences between programming languages and text and likely you'll need to syntax checkers for these anyway.

This pull request is still WIP (missing some tests and doc updates), but I'm opening it anyway for discussion.

/cc @cpitclaudel

@lunaryorn lunaryorn added this to the New syntax checker chaining milestone Sep 5, 2016

@lunaryorn lunaryorn referenced this pull request Sep 5, 2016

Open

Chain syntax checkers by stages #1072

7 of 11 tasks complete
@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 5, 2016

I ran a quick experiment and downloaded all packages whose name contained "flycheck". All of them had a :modes property, so I think this is safe :)

Some notes:

  • Many extensions have a single mode, but still use the list form: :modes (a-mode); other's don't use a list, though: :modes a-mode.
  • There was at least one wrong one:
(flycheck-define-checker cstyle
  "A checker using cstyle.

See `https://github.com/alexmurray/cstyle/'."
  :command ("cstyle"
            (config-file "--config" flycheck-cstyle-config)
            source)
  :error-patterns ((info line-start (file-name) ":" line ":" column ":"
                         (message (minimal-match (one-or-more anything)))
                         line-end))
  :modes c-mode c++-mode)

(reported at alexmurray/flycheck-cstyle#2)

flycheck.el Outdated
@@ -1598,13 +1593,10 @@ are mandatory.
If given this syntax checker is only used in buffers whose

This comment has been minimized.

@cpitclaudel

cpitclaudel Sep 5, 2016

Member

I think this should change, since the mode should always be given (I understand that the PR is still in flux :)

This comment has been minimized.

@lunaryorn

lunaryorn Sep 5, 2016

Contributor

Yup, thanks.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 5, 2016

@cpitclaudel Thanks a lot; that's good to know. I think the change is safe then.

Funny that folks have so many different styles :)

@lunaryorn lunaryorn force-pushed the require-major-mode branch 2 times, most recently from bdd0b6f to f347230 Sep 11, 2016

lunaryorn added some commits May 3, 2016

@lunaryorn lunaryorn force-pushed the require-major-mode branch from f347230 to bbd0829 Sep 11, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 11, 2016

@cpitclaudel Finished and ready for review 😊 For the record, LGTM.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 12, 2016

LGTM. IIRC buttercup has a feature to intercept function calls that could be used instead of cl-letf, but I don't think it would make a difference here :)

I like this new architecture. If a checkers want more mores, we could always introduce a different :modes spec, like (:derived-from <mode>), in the future.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 12, 2016

If a checkers want more mores, we could always introduce a different :modes spec, like (:derived-from ), in the future.

That's my idea.

Merged.

@lunaryorn lunaryorn merged commit cc04361 into master Sep 12, 2016

4 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@lunaryorn lunaryorn deleted the require-major-mode branch Sep 12, 2016

@radupopescu

This comment has been minimized.

radupopescu commented Sep 13, 2016

Unfortunately, this merge broke the flycheck-ycmd checker.

ptrv added a commit to ptrv/emacs-ycmd that referenced this pull request Sep 13, 2016

Add :modes to ycmd flycheck syntax checker
The :modes is not mandatory in syntax checker definition.
See: flycheck/flycheck#1071

Resolves abingham#351

ptrv added a commit to abingham/emacs-ycmd that referenced this pull request Sep 13, 2016

Add :modes to ycmd flycheck syntax checker
The :modes is now mandatory in syntax checker definition.
See: flycheck/flycheck#1071

Resolves #351
@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 13, 2016

@radupopescu

This comment has been minimized.

radupopescu commented Sep 13, 2016

I was just reporting this here, too, since this checker wasn't mentioned in this thread (it's part of the emacs-ycmd repo, it doesn't have it's own flycheck-***** repo).

Anyway, I think the maintainers of emacs-ycmd have made a fix today.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 13, 2016

Generally, if you're maintaining extensions to Flycheck outside of our organisation we expect you to follow our development process to be notified about breaking changes.

However we offer to move Flycheck extensions into the Flycheck organisation—for extensions and checkers in our org we notify maintainers of breaking changes and make necessary changes to restore compatibility.

@radupopescu

This comment has been minimized.

radupopescu commented Sep 13, 2016

Makes sense.

Are extensions part of the Flycheck organization allowed to require an external package/program to function?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 13, 2016

@radupopescu For sure… how would Flycheck work without external programs? 😉

@radupopescu

This comment has been minimized.

radupopescu commented Sep 13, 2016

@lunaryorn Yes, I guess it wasn't the best question. The flycheck package would be huge if it contained the logic for checking all these languages.

Thanks!

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 13, 2016

@radupopescu Sorry, I didn't mean to offend you—it was a stupid remark of mine 😞 I hope you don't bear a grudge against me 😌

If you've got an extension to move into Flycheck please feel free to ping us, either with an issue or on our gitter channel and we'll help you with moving the extension into our Org 😊

@radupopescu

This comment has been minimized.

radupopescu commented Sep 13, 2016

@lunaryorn No worries, no offense taken!

I don't personally maintain any extension, but I was trying to understand whether it would be technically feasable to include something like flycheck-ycmd into the flycheck package.

Thanks for all the information!

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment