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

[RDY] Remove jade checker and replace it with pug checker #1084

Merged
merged 1 commit into from Sep 15, 2016

Conversation

Projects
None yet
4 participants
@robbyoconnor
Contributor

robbyoconnor commented Sep 12, 2016

This removes the jade checker and adds the a checker for pug.

Closes GH-951

@CLAassistant

This comment has been minimized.

CLAassistant commented Sep 12, 2016

CLA assistant check
All committers have signed the CLA.

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch from 99946b3 to 8fdfafa Sep 12, 2016

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 12, 2016

@flycheck/maintainers @lunaryorn

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch from 8fdfafa to 6572ae4 Sep 12, 2016

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 12, 2016

Thanks, looks great :) (LGTM) Could you add a news entry, too?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Sep 12, 2016

(maybe the news entry should mention that the patterns have been updated, too?)

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 12, 2016

I'll fix the formatting snafu hold on

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 12, 2016

I'll fix that. Gotta let them know that this won't work as a drop-in for Jade. Do I need to update the texi file too in lagacy?

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch 3 times, most recently from e9b5395 to 6e6e189 Sep 12, 2016

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 12, 2016

I added f as a dependency in the Cask file. It broke make format without it.

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 12, 2016

Hold off -- I'm gonna keep the jade checker -- by using the pug checker...

Cask Outdated
@@ -19,13 +19,13 @@
(depends-on "d-mode")
(depends-on "erlang")
(depends-on "ess")
(depends-on "f")

This comment has been minimized.

@lunaryorn

lunaryorn Sep 12, 2016

Contributor

Please remove this line. Look above, f is already there…

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

It failed for me...

flycheck.el Outdated
"Error: Pug:" line ":" column (zero-or-more not-newline) "\n"
(one-or-more (or (zero-or-more not-newline) "|" (zero-or-more not-newline) "\n")
(zero-or-more "-") (zero-or-more not-newline) "|" (zero-or-more not-newline) "\n")
(zero-or-more not-newline) "\n" (one-or-more (zero-or-more not-newline) "|" (zero-or-more not-newline) "\n") (zero-or-more not-newline) "\n"

This comment has been minimized.

@lunaryorn

lunaryorn Sep 12, 2016

Contributor

Please wrap lines longer than 80 characters.

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

Okay :) This was the formatter's work actually...

@@ -3282,10 +3282,6 @@ Why not:
'(8 5 warning "discarding unexpected <spam>"
:checker html-tidy)))
(flycheck-ert-def-checker-test jade jade nil
(flycheck-ert-should-syntax-check
"language/jade.jade" 'jade-mode

This comment has been minimized.

@lunaryorn

lunaryorn Sep 12, 2016

Contributor

Did you remove the jade file as well?

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

I forgot :)

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

Handled this.

CHANGES.rst Outdated
@@ -157,6 +158,7 @@ version number. Breaking changes may occur at any point.
with ``go vet`` [GH-765] [GH-897]
- Add ``flycheck-ghc-stack-use-nix`` to enable Nix support for Stack GHC
[GH-913]
- Removal of the Jade checker [GH-951] [GH-1084]

This comment has been minimized.

@lunaryorn

lunaryorn Sep 12, 2016

Contributor

That's the wrong version to add the note to. Please rebase onto master and add the change notes to version 30 which is currently in development.

Furthermore, please keep the grammar and phrasing consistent to the other entries, i.e. not "Removal of", but just "remove".

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

Okay.

CHANGES.rst Outdated
@@ -136,6 +136,7 @@ version number. Breaking changes may occur at any point.
- Processing [GH-793] [GH-812]
- Racket [GH-799] [GH-873]
- Pug -- The pattern has been changed and this is not backwards compatible to Jade. [GH-951] [GH-1084]

This comment has been minimized.

@lunaryorn

lunaryorn Sep 12, 2016

Contributor

Please move this to the latest version and remove the explanatory sentence. It's enough for users to see that Jade was removed.

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

okay :)

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 12, 2016

Contributor

Done.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 12, 2016

@robbyoconnor Thanks a lot for your contribution.

Please don't keep the jade checker, though. If jade is deprecated there's no reason for us to keep it. Users that still depend on jade should either stick to an older version of Flycheck or manually resurrect the jade checker in their configuration.

There are also a couple of minor issues in the PR. Please address these and rebase onto master.

Thanks 👍

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch 8 times, most recently from f35a212 to 58abe05 Sep 12, 2016

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 12, 2016

All green! 💚

flycheck.el Outdated
(zero-or-more not-newline) "at "
(zero-or-more not-newline) " line " line)
;; syntax/runtime errors

This comment has been minimized.

@cpitclaudel

cpitclaudel Sep 15, 2016

Member

This scares me a bit: what kind of errors do you have in mind when you mention "runtime errors"?

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 15, 2016

Contributor

Let me fix that wording.

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 15, 2016

Contributor

runtime errors (e.g., type errors -- which aren't syntax errors)

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 15, 2016

Contributor

runtime error

TypeError: /home/rob/flycheck/test/resources/language/pug/pug-runtime-error.pug:5
    3|   head
    4|     body
  > 5|     = foo.bar
    6| 

Cannot read property 'bar' of undefined
    at eval (eval at wrap (/home/rob/.nvm/versions/node/v5.12.0/lib/node_modules/pug-cli/node_modules/pug-runtime/wrap.js:6:10), <anonymous>:13:60)
    at template (eval at wrap (/home/rob/.nvm/versions/node/v5.12.0/lib/node_modules/pug-cli/node_modules/pug-runtime/wrap.js:6:10), <anonymous>:13:137)
    at ReadStream.<anonymous> (/home/rob/.nvm/versions/node/v5.12.0/lib/node_modules/pug-cli/index.js:230:20)
    at emitNone (events.js:85:20)
    at ReadStream.emit (events.js:179:7)
    at endReadableNT (_stream_readable.js:913:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)```

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 15, 2016

Contributor

Not really a syntax error, more of a type error and yes these errors are verbose as hell and not fun to write regexps for...but...i've managed to make do :)

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch 2 times, most recently from 922b9a0 to a619704 Sep 15, 2016

@robbyoconnor robbyoconnor reopened this Sep 15, 2016

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 15, 2016

Sorry(triggering a CI build)

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 15, 2016

Build failures aren't my fault if they occur...seems travis times out

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch from a619704 to 46a607c Sep 15, 2016

@lunaryorn

Thanks for the changes. There are couple of stylistic issues left, and the tests need to be split into individual test cases.

flycheck.el Outdated
((error "Error: " (message) (zero-or-more not-newline) "\n"
(zero-or-more not-newline) "at "
(zero-or-more not-newline) " line " line)

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please remove this blank line.

flycheck.el Outdated
(zero-or-more not-newline) "\n") (zero-or-more not-newline) "\n"
(message) line-end))

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please remove this blank lines as well.

@@ -3547,6 +3543,17 @@ Why not:
"language/processing/syntax_error/syntax_error.pde" 'processing-mode
'(4 2 error "Syntax error, maybe a missing semicolon?"
:checker processing)))
(flycheck-ert-def-checker-test pug pug nil

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please add a blank line above.

(flycheck-ert-should-syntax-check
"language/pug/pug.pug" 'pug-mode
'(2 1 error "unexpected token \"indent\"" :checker pug))
(flycheck-ert-should-syntax-check

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please split the test into multiple test cases. There should only be one -ert-should-syntax-check call per -ert-def-checker-test.

The 3rd argument—which is nil currently—let's you change give a name to each checker test.

@@ -1,6 +1,6 @@
doctype html
html
head
title Jade Examples
title Pug Examples

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please move this file into the pug/ directory to put it beneath all the other pug files.

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 15, 2016

Contributor

I thought i did... weird

flycheck.el Outdated
(one-or-more (and (zero-or-more not-newline) "|"
(zero-or-more not-newline) "\n"))
(zero-or-more not-newline) "\n" (message) line-end))
:modes jade-mode)

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please remove this blank line.

This comment has been minimized.

@robbyoconnor

robbyoconnor Sep 15, 2016

Contributor

What? There is no blank line here...

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

You removed the code only but no adjacent blank line, so there were to blank lines between the previous and the next checker now. Looks fixed in the current diff.

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch 2 times, most recently from 05b467c to 2801e9d Sep 15, 2016

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 15, 2016

Made the changes.

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch from 2801e9d to 288a150 Sep 15, 2016

@lunaryorn

Another bunch of minor style issues and typos, but other than that LGTM. Please fix those and I'll merge.

Thanks a lot for this contributions. Awesome work!

flycheck.el Outdated
((error "Error: " (message) (zero-or-more not-newline) "\n"
(zero-or-more not-newline) "at "
(zero-or-more not-newline) " line " line)
;; syntax/runtime errors(e.g, type errors)

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

There's a space missing before the opening parenthesis, and the comma after e.g should be a dot :)

flycheck.el Outdated
:command ("pug" "-p" (eval (expand-file-name (buffer-file-name))))
:standard-input t
:error-patterns
;; errors with includes/extends (e.g., missing files)

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

There's a redundant comma

(flycheck-ert-should-syntax-check
"language/pug/pug-extends.pug" 'pug-mode
'(1 nil error
"the \"basedir\" option is required to use includes and extends with \"absolute\" paths" :checker pug)))

This comment has been minimized.

@lunaryorn

lunaryorn Sep 15, 2016

Contributor

Please move the message to the line before, and instead break before :checker. That's how we did it in the other tests, I find that it's easier to read this way.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Sep 15, 2016

LGTM, because LGTM.co didn't seem to catch the review comment.

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch 3 times, most recently from 17fd914 to 515efb3 Sep 15, 2016

@robbyoconnor

This comment has been minimized.

Contributor

robbyoconnor commented Sep 15, 2016

Additional changes have been made.

Remove jade checker and replace it with pug checker
Jade is now deprecated and is replaced by pug. I do not feel it is
worth while to maintain a checker for both.

Closes GH-951

@robbyoconnor robbyoconnor force-pushed the robbyoconnor:add-pug-mode-checker branch from 515efb3 to 7aacef9 Sep 15, 2016

@lunaryorn lunaryorn merged commit 2875c63 into flycheck:master Sep 15, 2016

3 checks passed

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

This comment has been minimized.

Contributor

lunaryorn commented Sep 15, 2016

Many many thanks 👍

@robbyoconnor robbyoconnor deleted the robbyoconnor:add-pug-mode-checker branch Sep 15, 2016

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