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 flymake-checkers #331

Merged
merged 1 commit into from
Oct 11, 2012
Merged

Add flymake-checkers #331

merged 1 commit into from
Oct 11, 2012

Conversation

swsnr
Copy link
Contributor

@swsnr swsnr commented Oct 11, 2012

flymake-checkers provides additional syntax checkers for flymake.

Currently there are checkers for Emacs Lisp, Python, Ruby, CoffeeScript and LaTeX.

flymake-checkers provides additional syntax checkers for flymake.

See https://github.com/lunaryorn/flymake-checkers for details
@purcell
Copy link
Member

purcell commented Oct 11, 2012

How does this differ from flymake-ruby, flymake-coffee, flymake-python-flymakes etc.? I maintain those packages, and I'd rather see them improved than replaced by a single package which tries to do it all.

@swsnr
Copy link
Contributor Author

swsnr commented Oct 11, 2012

  • Different or more tools (flake8 and pylint in addition to pyflakes, coffeelint instead of coffee)
  • Different approach on extending flymake: It adds all checkers to flymake-allowed-file-name-masks and let's flymake do the rest. I think that's more how flymake is intended to be extended, and it's compatible with flymake being globally enabled by (add-hook 'visit-file-hook 'flymake-visit-file-hook) as described in the flymake manual.

I do not think that maintaining separate packages for each flymake checker is a good idea. Consider how many syntax checkers syntastic provides. A separate package for each of these will be a maintenance and installation nightmare. Already you have duplicated functionality between all your flymake packages, i.e. the temp-dir function, or the advice hack you're using to suppress fatal error messages.

Instead of going down that road, I'd rather merge all your flymake checkers into this package and then start stealing from syntastic to add more checkers.

@purcell
Copy link
Member

purcell commented Oct 11, 2012

The choices made by a vim plugin don't automatically make a compelling argument, but I understand your point. :-)

I'm not committed to keeping separate flymake-* libraries, but I'm genuinely interested in the trade-offs, and I'd like to know that I could switch to flymake-checkers myself if that is the best choice!

I agree that the flymake-allowed-file-name-masks scheme is the intended way for flymake to be activated, but when initially using that scheme I experienced many cases where ruby-mode files (for example) would not have flymake enabled, because the full exhaustive list of Rakefile, Gemfile filenames etc. had not been added to the file-name-masks list. I finally concluded that it's redundant and undesirable to maintain two separate lists of file name patterns for the same basic purpose, but YMMV.

flymake hasn't changed in years, and I'm not entirely convinced that it is well-designed: for example, everyone seems to use that advice hack.

purcell added a commit that referenced this pull request Oct 11, 2012
@purcell purcell merged commit e19c7a7 into melpa:master Oct 11, 2012
@purcell
Copy link
Member

purcell commented Oct 11, 2012

In any case, I've merged this recipe. Feel free to post any further thoughts here, though.

@purcell
Copy link
Member

purcell commented Oct 11, 2012

In terms of the duplication between flymake-ruby, flymake-coffee et al., it would equally be possible (and indeed desirable) to extract the commonality into a separate library which could then be used by future flymake-tex etc.

In other words, the presence of duplicated code doesn't itself imply that everything should go into a single library.

@swsnr
Copy link
Contributor Author

swsnr commented Oct 11, 2012

I aim to provide syntax checking that works out of the box with minimal or no upfront configuration. Reyling on flymake-allowed-file-name-masks I get close to this, because (add-hook 'find-file-hook 'flymake-find-file-hook) is enough to make flymake work.

I do agree, though, that flymake-allowed-file-name-masks essentially duplicates auto-mode-alist. I'll get rid of it, and use something more flexible. I'm already experimenting with code modelled after syntastic's way of dealing with syntax checkers. I'll also try to cut back flymake's ugliness, especially this mucking around with flymake-init-create-temp-buffer-copy. In the end, I'll likely replace most of flymakes configuration stuff, hopefully making it easier to add new checkers or port existing ones.

flymake is not well-designed, surely, but various workarounds, checker implementations and hacks seen in the wild are neither. I fail to see the purpose of the advice hack, or what advantages it should have over (setq flymake-gui-warnings-enabled nil flymake-log-level -1).

@purcell
Copy link
Member

purcell commented Oct 11, 2012

+1. I think it makes sense to leverage as much as possible of flymake because the core behaviour of "run this, look for errors matching this pattern, then display them as overlays" is basically well implemented and tested. I've seen a few other syntax-checking libs which do not use flymake, but I have generally assumed that the author hadn't heard of flymake. :-)

It looks like the defadvice is required in order to avoid hard errors in certain circumstances, at least as far as I can see from my commit messages... I lived without the defadvice for a while, then had to add it to all my flymake-* libs.

Here are some other checkers you might like to incorporate:

https://github.com/purcell/flymake-haml
https://github.com/purcell/flymake-sass
https://github.com/purcell/flymake-php
https://github.com/purcell/flymake-jslint
https://github.com/purcell/flymake-css

:-)

@swsnr
Copy link
Contributor Author

swsnr commented Oct 12, 2012

@purcell I do intend to reuse as much of flymake as possible, but some parts just suck. I will replace the configuration mechanism, and the way checkers are written, because I find flymake's way of doing things clumsy, especially if the checker needs a custom error pattern.

Once that is done, I'll merge all of your flymake checkers into the package. I'll get in touch again, if that is done.

@purcell
Copy link
Member

purcell commented Oct 12, 2012

Awesome -- I'm looking forward to it!

milkypostman pushed a commit that referenced this pull request Oct 12, 2012
@swsnr
Copy link
Contributor Author

swsnr commented Oct 12, 2012

@purcell In lunaryorn/flymake-checkers#2 I've come up with a new, more declarative checker API and an alternative way of registering checkers, based on the current major mode. I'm quite happy with the result, but I'd like to hear your opinion.

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