Skip to content

[Fix #305] Don't call lookup-alias for keywords #311

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 29, 2016
Merged

[Fix #305] Don't call lookup-alias for keywords #311

merged 1 commit into from
Mar 29, 2016

Conversation

benedekfazekas
Copy link
Member

No description provided.

expez
expez reviewed Mar 21, 2016
View reviewed changes
clj-refactor.el
@@ -2199,6 +2199,12 @@ the alias in the project."
(-when-let (long (cljr--aget cljr-magic-require-namespaces short))
(list short (list long))))))))

(defun cljr--in-keyword-p ()
(cljr--is-keyword?
(buffer-substring-no-properties
Copy link
Member

Choose a reason for hiding this comment

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

The argument here is just cider-symbol-at-point.

Since we already have cljr--is-keyword? perhaps we should make s optional and act on the symbol at point otherwise? I also think we should rename cljr--is-keyword ro just cljr--keyword? so it makes what we have everywhere else.

expez
expez reviewed Mar 22, 2016
View reviewed changes
clj-refactor.el
@@ -2199,6 +2200,12 @@ the alias in the project."
(-when-let (long (cljr--aget cljr-magic-require-namespaces short))
(list short (list long))))))))

(defun cljr--in-sans-ns-keyword-p ()
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike this name. I think a better name would be cljr--in-keyword-sans-alias?. We're using ? for predicates everywhere else, so we should be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

i am not married to this name. cljr--in-keyword-sans-alias? it is then.

apparently it is consistent with the cider naming (already ;) ):

(not (cider-in-comment-p))
(not (cider-in-string-p))

not married to this either tho. happy to rename if you insist.

Copy link
Member

Choose a reason for hiding this comment

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

This function should probably have some docstring.

As for predicates - you should really adopt the Emacs conventions as you're writing Emacs Lisp, not Clojure. @magnars is one of the few Elisp hackers who uses ?, but in dash.el even he added the traditional names as aliases. I profoundly dislike violating established conventions for no good reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok I prefer the -p style preds. But really don't want to go down the rabbit hole of renaming all our foo? style preds in this PR.

  • will create a separate PR replacing all our own ? predicates with -p ones
  • keep this fn -p style

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me. I was merely pointing out a related observation.

On 25 March 2016 at 20:49, Benedek Fazekas notifications@github.com wrote:

In clj-refactor.el
#311 (comment)
:

@@ -2199,6 +2200,12 @@ the alias in the project."
(-when-let (long (cljr--aget cljr-magic-require-namespaces short))
(list short (list long))))))))

+(defun cljr--in-sans-ns-keyword-p ()

ok I rather prefer the -p style preds. But really don't want to go down
the rabbit hole of renaming all our foo? style preds in this PR.

  • will create a separate PR replacing all our own ? predicates with -p
    ones
  • keep this fn -p style

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/clojure-emacs/clj-refactor.el/pull/311/files/5f2adca5f6175d34508f83ea951cb48356fbb4c0#r57476657

Best Regards,
Bozhidar Batsov

http://www.batsov.com

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for doing that @bbatsov :)

@benedekfazekas benedekfazekas force-pushed the fix-305 branch 2 times, most recently from 6da1995 to 02c7a97 Compare March 25, 2016 19:52
benedekfazekas added a commit that referenced this pull request Mar 26, 2016
@benedekfazekas
[refactor] Use Elisp style, -p predicates
b6bff9f 
As discussed in #311 `-p` style predicates
are more consistent with the rest of clojure-emacs projects and the
language in general.

cljr predicates renamed

- cljr--use-multiple-cursors?
- cljr--comment-line?
- cljr--whitespace?
- cljr--inside-prefixed-libspec-vector?
- cljr--even?
- cljr--op-supported?
- cljr--cljs-file?
- cljr--cljc-file?
- cljr--clj-file?
- cljr--dash-in-file-name?
- cljr--already-declared?
- cljr--existing-group-of-defs?
- cljr--point-in-reader-conditional?
- cljr--point-in-reader-conditional-branch?
- cljr--clj-context?
- cljr--in-namespace-declaration?
- cljr--excluded-from-project-clean?
- cljr--empty-buffer?
- cljr--end-of-buffer?
- cljr--qualified-symbol?
- cljr--is-keyword?
- cljr--symbol?
- cljr--keyword-lookup?
- cljr--defn?
- cljr--call-site?
- cljr--no-changes-to-parameter-order?
- cljr--apply-call-site?
- cljr--partial-call-site?
- cljr--ignorable-occurrence?

predicates renamed from dependencies

- -contains? (dash)
- -every? (dash)
- s-contains? (s)
- s-starts-with? (s)
- s-ends-with? (s)
- s-equals (s)
- s-matches? (s)

predicates not renamed as there is no suitable alias

- s-blank? (s)
- s-present? (s)
- s-uppercase? (s)
@benedekfazekas benedekfazekas mentioned this pull request Mar 26, 2016
Merged
@expez
Copy link
Member

expez commented Mar 29, 2016

This looks good now! 👍

@benedekfazekas
[Fix #305] Handle keywords in cljr-slash
215e938 
Don't call lookup-alias for non namespaced keywords at all when slash is
typed. However trigger lookup alias with the leading :: stripped off the
prefix if the keyword is namespaced.

- fix bug in `cljr--keyword?` so it recognises aliased, `::` keywords
@benedekfazekas benedekfazekas merged commit 28788fd into master Mar 29, 2016
@benedekfazekas benedekfazekas deleted the fix-305 branch March 29, 2016 09:40
benedekfazekas added a commit that referenced this pull request Mar 29, 2016
@benedekfazekas
[refactor] Use Elisp style, -p predicates
b636338 
As discussed in #311 `-p` style predicates
are more consistent with the rest of clojure-emacs projects and the
language in general.

cljr predicates renamed

- cljr--use-multiple-cursors?
- cljr--comment-line?
- cljr--whitespace?
- cljr--inside-prefixed-libspec-vector?
- cljr--even?
- cljr--op-supported?
- cljr--cljs-file?
- cljr--cljc-file?
- cljr--clj-file?
- cljr--dash-in-file-name?
- cljr--already-declared?
- cljr--existing-group-of-defs?
- cljr--point-in-reader-conditional?
- cljr--point-in-reader-conditional-branch?
- cljr--clj-context?
- cljr--in-namespace-declaration?
- cljr--excluded-from-project-clean?
- cljr--empty-buffer?
- cljr--end-of-buffer?
- cljr--qualified-symbol?
- cljr--is-keyword?
- cljr--symbol?
- cljr--keyword-lookup?
- cljr--defn?
- cljr--call-site?
- cljr--no-changes-to-parameter-order?
- cljr--apply-call-site?
- cljr--partial-call-site?
- cljr--ignorable-occurrence?

predicates renamed from dependencies

- -contains? (dash)
- -every? (dash)
- s-contains? (s)
- s-starts-with? (s)
- s-ends-with? (s)
- s-equals (s)
- s-matches? (s)

predicates with extra hypen

- cljr--toplevel-p
- cljr--whitespace-p
- cljr--defn-p
- cljr--asts-p

predicates not renamed as there is no suitable alias

- s-blank? (s)
- s-present? (s)
- s-uppercase? (s)
benedekfazekas added a commit that referenced this pull request Mar 29, 2016
@benedekfazekas
[refactor] Use Elisp style, -p predicates
c79ff74 
As discussed in #311 `-p` style predicates
are more consistent with the rest of clojure-emacs projects and the
language in general.

cljr predicates renamed

- cljr--use-multiple-cursors?
- cljr--comment-line?
- cljr--whitespace?
- cljr--inside-prefixed-libspec-vector?
- cljr--even?
- cljr--op-supported?
- cljr--cljs-file?
- cljr--cljc-file?
- cljr--clj-file?
- cljr--dash-in-file-name?
- cljr--already-declared?
- cljr--existing-group-of-defs?
- cljr--point-in-reader-conditional?
- cljr--point-in-reader-conditional-branch?
- cljr--clj-context?
- cljr--in-namespace-declaration?
- cljr--excluded-from-project-clean?
- cljr--empty-buffer?
- cljr--end-of-buffer?
- cljr--qualified-symbol?
- cljr--is-keyword?
- cljr--symbol?
- cljr--keyword-lookup?
- cljr--defn?
- cljr--call-site?
- cljr--no-changes-to-parameter-order?
- cljr--apply-call-site?
- cljr--partial-call-site?
- cljr--ignorable-occurrence?

predicates renamed from dependencies

- -contains? (dash)
- -every? (dash)
- s-contains? (s)
- s-starts-with? (s)
- s-ends-with? (s)
- s-equals (s)
- s-matches? (s)

predicates with extra hypen

- cljr--toplevel-p
- cljr--whitespace-p
- cljr--defn-p
- cljr--asts-p

predicates not renamed as there is no suitable alias

- s-blank? (s)
- s-present? (s)
- s-uppercase? (s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benedekfazekas @expez @bbatsov