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 initial support for Ruumba. #1616

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@aclemons
Copy link
Contributor

commented Sep 1, 2019

Ruumba is a tool for RuboCop linting of ERB templates.

https://github.com/ericqweinstein/ruumba

Closes #1615

@CLAassistant

This comment has been minimized.

Copy link

commented Sep 1, 2019

CLA assistant check
All committers have signed the CLA.

@fmdkdd
Copy link
Member

left a comment

Thanks! The checker definition looks good. Just a couple minor points.

Could you write a test for it as well?

flycheck.el Outdated
Otherwise report style issues as well."
:safe #'booleanp
:type 'boolean
:package-version '(flycheck . "0.16"))

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Sep 2, 2019

Member

Version should be 32:

Suggested change
:package-version '(flycheck . "0.16"))
:package-version '(flycheck . "32"))

This comment has been minimized.

Copy link
@aclemons

aclemons Sep 2, 2019

Author Contributor

I've updated this now

flycheck.el Outdated
(optional (id (one-or-more (not (any ":")))) ": ") (message)
line-end))
:modes (html-erb-mode rhtml-mode)
:next-checkers ((warning . eruby-erubis)))

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Sep 2, 2019

Member

So you want to erubis to run after ruumba, but since you added eruby-ruumba after eruby-erubis in the flycheck-checkers list, erubis will remain the default checker in ERB and RHTML buffers. Is this intended?

This comment has been minimized.

Copy link
@aclemons

aclemons Sep 2, 2019

Author Contributor

Hmm, my intention was to not change any defaults, just to add ruumba after erubis. I have reversed the order, adding next-checker to eruby-erubis instead.

@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Thanks! The checker definition looks good. Just a couple minor points.

Could you write a test for it as well?

I'll have a go at getting an integration test running

@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

I'll open a PR for https://github.com/flycheck/docker-tools/ to add ruumba to the docker image?

@aclemons aclemons referenced this pull request Sep 2, 2019
@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

PR opened for that now
flycheck/docker-tools#4

@aclemons aclemons force-pushed the aclemons:ruumba branch from ab552e8 to 770092d Sep 2, 2019

@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

I've added an initial integration test now.

@aclemons aclemons force-pushed the aclemons:ruumba branch from 770092d to 9ba60a2 Sep 3, 2019

@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Pushed to retrigger build

@fmdkdd
fmdkdd approved these changes Sep 3, 2019
Copy link
Member

left a comment

Thank you for the swift turnaround. LGTM.

@@ -3316,9 +3316,18 @@ See https://github.com/flycheck/flycheck/issues/531 and Emacs bug #19206"))
"language/erlang/rebar3/_build/default/lib/dependency/_build")))))

(flycheck-ert-def-checker-test eruby-erubis eruby nil
(let ((flycheck-disabled-checkers '(eruby-ruumba)))
(flycheck-ert-should-syntax-check

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Sep 3, 2019

Member

You want to re-indent the let body to pass the CI.

This comment has been minimized.

Copy link
@aclemons

aclemons Sep 3, 2019

Author Contributor

Sorry, I think I have them properly formatted now.


(flycheck-ert-def-checker-test eruby-ruumba eruby syntax-error
(let ((flycheck-disabled-checkers '(eruby-erubis)))
(flycheck-ert-should-syntax-check

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Sep 3, 2019

Member

and re-indent here as well.

This comment has been minimized.

Copy link
@aclemons

aclemons Sep 3, 2019

Author Contributor

And here too.

@aclemons aclemons force-pushed the aclemons:ruumba branch from 9ba60a2 to 2f2c035 Sep 3, 2019

@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I've squashed those fixes now also.

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Thanks! The remaining CI failures are due to unrelated checker updates in the Docker image . I'll fix that tomorrow and you'll be able to rebase.

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@aclemons Done. You can rebase on master.

@aclemons aclemons force-pushed the aclemons:ruumba branch 2 times, most recently from 947d526 to cd9da80 Sep 4, 2019

@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

It is failing now JOB="emacs" EMACS_VERSION="24.5" but this looks to be some kind of flake. Installing the cask dependencies is failing.

[...]
  - Installing [23/48] nix-mode (latest)... downloadingMakefile:92: recipe for target 'init' failed
make: *** [init] Error 255

The other two are passing (Emacs 25.3 / 26.2)

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Not a fluke: latest nix-mode now requires Emacs 25. This happened between my build and yours, as it turned out!

I've made a patch. After I check it passes CI, I'll merge and you'll be able to rebase again (hopefully for the last time!).

@fmdkdd

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

I fixed the issue in master. Good to go.

@aclemons aclemons force-pushed the aclemons:ruumba branch from cd9da80 to e1804d7 Sep 5, 2019

Add initial support for Ruumba.
Ruumba is a tool for RuboCop linting of ERB templates.

https://github.com/ericqweinstein/ruumba

Closes #1615
@aclemons

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Ok, rebased now

@fmdkdd fmdkdd merged commit eef4903 into flycheck:master Sep 6, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@fmdkdd

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Thanks! And nice work 👍 .

@aclemons aclemons deleted the aclemons:ruumba branch Sep 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.