Introduce completable-for-cljr-slash?
#493
Conversation
vemv
force-pushed
the
492--completable-for-cljr-slash
branch
from
d7679e8
to
642dd4a
Compare
|
Ready for re-review |
vemv
force-pushed
the
492--completable-for-cljr-slash
branch
from
7d15089
to
5134e25
Compare
| @@ -1983,37 +1988,47 @@ the alias in the project." | |||
| (backward-sexp 1) | |||
| (looking-at-p "[-+0-9]"))) | |||
|
|
|||
| (defun completable-for-cljr-slash? (sym) | |||
There was a problem hiding this comment.
All definitions should have a docstring, especially the "public" ones.
| (when sym | ||
| (or | ||
| ;; vanilla symbols, maybe prefixed by ^, :: or ^:: | ||
| (not (null (string-match-p "^\\^?\\(::\\)?\\([a-zA-Z]+[a-zA-Z0-9\\-]*\\)+\\(\\.?[a-zA-Z]+[a-zA-Z0-9\\-]*\\)*$" sym))) |
There was a problem hiding this comment.
Seems to me you can rewrite this code with the not null check and it'd read better.
There was a problem hiding this comment.
I cargo-culted the pattern from https://github.com/magnars/s.el/blob/08661efb075d1c6b4fa812184c1e5e90c08795a9/s.el#L281-L283 (s--truthy? is used a few times there, so transitively the not-null check is used a bunch of times there)
No strong opinion from my side, but maybe if in doubt we can do as s.el does
There was a problem hiding this comment.
I can live with this, it just seems quite verbose.
There was a problem hiding this comment.
I don't think these regexps are correct - Clojure symbols allow much more than alphanumerics.
Probably can be rewritten with the help of clojure--sym-regexp
There was a problem hiding this comment.
Probably can be rewritten with the help of clojure--sym-regexp
Good idea! Will keep in mind.
Will also leave this PR on hold for a bit - not good to mix old with possibly newly-introduced bugs
| @@ -9,11 +9,13 @@ default: &default-steps | |||
| - run: make elpa | |||
| - run: emacs --version | |||
| - run: cask --version | |||
| - run: make unit | |||
There was a problem hiding this comment.
I think a target like unit-tests would be better here. make test should probably run all tests and there should be a separate make target for the integration tests.
| (describe "completable-for-cljr-slash?" | ||
| (it "Returns `t' for tokens that can represent alias-prefixed named things (symbols, keywords, metadata)" | ||
| (expect (completable-for-cljr-slash? nil) :to-be nil) | ||
| (dolist (prefix '("" |
There was a problem hiding this comment.
What about #'my-alias/private-var?
Think that's the only other form that I use regularly that isn't covered.
There was a problem hiding this comment.
Good call! backtick also
(unrelated, I think some CIDER features don't parse #' or backtick either...)
| (string-remove-prefix "^::") | ||
| (string-remove-prefix "::") | ||
| (string-remove-prefix "^") | ||
| (string-remove-prefix "@"))) |
|
Seems we forgot about this one. |
|
Will give it some Elisp love soon enough :) |
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
Reflects the decisions we made over #493. Code has been copied as is, except for the unit tests in question, which are for `cljr--ns-name` (as a random example that I picked).
|
Will re-do at a later point. |
Fixes #492