Skip to content

Fix namespace font locking #448

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

Closed
wants to merge 4 commits into from
Closed

Fix namespace font locking #448

wants to merge 4 commits into from

Conversation

Bost
Copy link
Contributor

@Bost Bost commented Oct 2, 2017

  • The commits are consistent with our [contribution guidelines][1]

  • You've added tests (if possible) to cover your change(s). Indentation & font-lock tests are extremely important!

  • All tests are passing (make test)

  • Using cask for testing is a pain :-(

  1. Cask installation doesn't work from behind a proxy
  2. Being behind or a proxy or not - in both cases I get:
$ make test
make: *** No rule to make target '/home/bost/dev/clojure-mode/.cask/25.3/elpa', needed by 'test'.  Stop.

However M-x spacemacs/ert-run-tests-buffer shows no new errors (on top of those 8 existing already)

  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)
  • I fixed a bug. No readme change is needed.

[CamelCase]))

;; examples of valid namespace definitions
(comment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually also add new unit tests verifying the validity of your changes.

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@
* [#443](https://github.com/clojure-emacs/clojure-mode/issues/443): Fix behavior of `clojure-forward-logical-sexp` and `clojure-backward-logical-sexp` with conditional macros.
* [#429](https://github.com/clojure-emacs/clojure-mode/issues/429): Fix a bug causing last occurrence of expression sometimes is not replaced when using `move-to-let`.
* [#423](https://github.com/clojure-emacs/clojure-mode/issues/423): Make `clojure-match-next-def` more robust against zero-arity def-like forms.
* Fix namespace font-locking
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also mention here the GH issue you've fixed. This sentence should end with a ..

Copy link
Contributor Author

@Bost Bost Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the fix out of pure excitement of having found "a pristine spot on a wall I can spray my tag on". Ok seriously there is no GH issue for my PR. Although the #280 seems to be closely related.

@bbatsov
Copy link
Member

bbatsov commented Oct 12, 2017

However M-x spacemacs/ert-run-tests-buffer shows no new errors (on top of those 8 existing already)

Hmm, that's odd. There shouldn't be any failing tests (there were certainly none last time I checked). Unfortunately the TravisCI setup seems to have been magically broken and I don't have time to look into this myself at this time.

@Bost
Copy link
Contributor Author

Bost commented Oct 12, 2017

Hmm, that's odd. There shouldn't be any failing tests (there were certainly none last time I checked).

It seems like something's messed up somewhere also on my side. M-x spacemacs/ert-run-tests-buffer on current master 35f5d71 now gives me only 4 errors. Not 8 as I reported. And my changes introduced 4 new errors on top of the 4 from the master. Merde :-( Could you please run the test on your machine? Thx.

Here the M-x spacemacs/ert-run-tests-buffer of test/clojure-mode-font-lock-test.el on current master 35f5d71:

Selector: (satisfies (lambda (test) (eq cbuf (spacemacs//find-ert-test-buffer test))))
Passed:  30
Failed:  4 (4 unexpected)
Skipped: 0
Total:   34/34

Started at:   2017-10-13 01:07:49+0200
Finished.
Finished at:  2017-10-13 01:07:52+0200

F.F.......F.............F.........

F clojure-mode-syntax-table/characters
    (ert-test-failed
     ((should
       (eq
        (clojure-test-face-at 1 2 "\\1")
        'clojure-character-face))
      :form
      (eq various-faces clojure-character-face)
      :value nil))

F clojure-mode-syntax-table/comment-macros
    (ert-test-failed
     ((should
       (equal
        (clojure-test-face-at 5 41 "#_ 
;; some crap
 (lala 0101
 lao

 0 0i)")
        'font-lock-comment-face))
      :form
      (equal various-faces font-lock-comment-face)
      :value nil :explanation
      (different-atoms various-faces font-lock-comment-face)))

F clojure-mode-syntax-table/fn
    (ert-test-failed
     ((should
       (eq
        (clojure-test-face-at 2 3)
        'font-lock-keyword-face))
      :form
      (eq t font-lock-keyword-face)
      :value nil))

F clojure-mode-syntax-table/ns-macro
    (ert-test-failed
     ((should
       (eq
        (clojure-test-face-at 1 10 "[ns name]")
        nil))
      :form
      (eq various-faces nil)
      :value nil))

@Bost
Copy link
Contributor Author

Bost commented Oct 17, 2017

I'm closing this PR. I'm gonna send you a new PR without regressions.

@Bost Bost closed this Oct 17, 2017
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