Fix for namespaced keywords #83

vmfhrmfoaj · 2022-03-26T11:55:18Z

First of all, thank you for developing and maintaining 'compliment'.
I found problems related to namespaced keywords:

compliment.context/try-read-replacing-maps return nil if aliased namespace keywords(e.g. ::my-ns/keyword) are contained in context string. (read-str try to resolve the alias)
I changed try-read-replacing-maps to replace aliased namespace keyword to namespaced keyword(e.g. :my-ns/keyword) to avoid the problem.
compliment.sources.local-bindings/parse-binding ignore variables that destructuring the map with namespaced keywords:
```
(require '[instaparse.gll :as gll])
(let [#_..., {::gll/keys [start-index end-index]} (meta x)])
```
I changed parse-binding to handle namespaced keywords.

vemv

Some suggestions

vemv · 2022-03-26T12:03:17Z

src/compliment/context.clj

@@ -18,6 +18,7 @@
         (-> s
             (str/replace "{" "(compliment-hashmap ")
             (str/replace "}" ")")
+             (str/replace #"::([-\w]+/)" (fn [[_ kw-ns]] (str ":" kw-ns)))


It seems more accurate to query (ns-aliases *ns*) to obtain a namespace name to be prepended to kw-ns?

vemv · 2022-03-26T12:08:06Z

test/compliment/sources/t_local_bindings.clj

-                                                {:keys [key1 key2] :strs [key3]} m2
+                                                {:keys [key1 key2] :strs [key3] :syms [key4]} m2
+                                                {:keys [:a/key5 :b/key6]} m3
+                                                {:c/keys [key7 key8]} m4


Shouldn't there also be test coverage for...?

:c/syms

::c/keys

::c/syms

vemv · 2022-03-26T12:09:08Z

test/compliment/sources/t_local_bindings.clj

@@ -82,10 +82,13 @@
                                         '(let [foo 42,
                                                [bar baz] lst
                                                {a :a, {b :b :as c} :b, [d] :d} m
-                                                {:keys [key1 key2] :strs [key3]} m2
+                                                {:keys [key1 key2] :strs [key3] :syms [key4]} m2
+                                                {:keys [:a/key5 :b/key6]} m3


I think there should also be coverage for {:keys [::a/keyN ::b/keyM]} (i.e. ns-aliased)

vemv · 2022-03-26T12:10:38Z

src/compliment/sources/local_bindings.clj

@@ -12,6 +12,10 @@

 (def letfn-like-forms '#{letfn})

+(def destructuring-keys #{:keys :strs :syms})
+
+(def destructuring-key-names #{"keys" "strs" "syms"})


Suggested change

(def destructuring-key-names #{"keys" "strs" "syms"})

(def destructuring-key-names (into #{} (map name) destructuring-keys))

vmfhrmfoaj · 2022-03-26T15:50:58Z

@vemv Fixed, thank you for review.

bbatsov · 2022-06-17T07:55:07Z

@vmfhrmfoaj Your changes look to good to me. Can you also mention them in the changelog, please?

vmfhrmfoaj · 2022-06-17T08:53:32Z

@bbatsov I updated.

vemv · 2022-06-17T08:57:01Z

Thanks for accurately satisfying all suggestions!

bbatsov · 2022-06-17T09:14:59Z

@vmfhrmfoaj Can you also rebase on top master to resolve the conflict in the changelog?

bbatsov · 2022-06-17T09:15:39Z

CHANGELOG.md

@@ -2,6 +2,9 @@

 ### master (unreleased)

+- Complete local bindings declared in map destructuring with namespaced keywords.


Please, prefix those entries with the link to the PR.

… generating the context `read-string` try to resolve an alias of namespaced keyword and throw 'invalid token' exception if there is no the alias. so, `try-read-replacing-maps` return `nil` if the context is contained the aliased namespace keyword. (except if alias is already added to *ns*) Benchmark before: Evaluation count : 126 in 6 samples of 21 calls. Execution time mean : 5.085000 ms Execution time std-deviation : 141.272913 µs Execution time lower quantile : 4.982329 ms ( 2.5%) Execution time upper quantile : 5.293605 ms (97.5%) Overhead used : 5.443103 ns -------------------------------------------------------------------------------- Benchmark after, string ver `(str/replace "::" ":")`: Evaluation count : 126 in 6 samples of 21 calls. Execution time mean : 5.219457 ms Execution time std-deviation : 213.306958 µs Execution time lower quantile : 5.046182 ms ( 2.5%) Execution time upper quantile : 5.456526 ms (97.5%) Overhead used : 6.702279 ns -------------------------------------------------------------------------------- Benchmark after, regular expression ver: Evaluation count : 120 in 6 samples of 20 calls. Execution time mean : 5.265777 ms Execution time std-deviation : 208.308093 µs Execution time lower quantile : 5.122747 ms ( 2.5%) Execution time upper quantile : 5.522446 ms (97.5%) Overhead used : 5.454686 ns

if use namespaced keywords, can't complete destructured variables Before benchmark: Evaluation count : 126 in 6 samples of 21 calls. Execution time mean : 5.070952 ms Execution time std-deviation : 86.793704 µs Execution time lower quantile : 5.011867 ms ( 2.5%) Execution time upper quantile : 5.208428 ms (97.5%) Overhead used : 5.254489 ns -------------------------------------------------------------------------------- After benchmark: Evaluation count : 120 in 6 samples of 20 calls. Execution time mean : 5.110484 ms Execution time std-deviation : 139.512617 µs Execution time lower quantile : 4.972072 ms ( 2.5%) Execution time upper quantile : 5.263425 ms (97.5%) Overhead used : 6.469988 ns

bbatsov · 2022-06-17T09:39:35Z

Thanks!

vemv reviewed Mar 26, 2022

View reviewed changes

vmfhrmfoaj force-pushed the master branch from 04bc3a2 to 2fc5068 Compare March 26, 2022 15:48

vmfhrmfoaj force-pushed the master branch from 2fc5068 to 12ec807 Compare June 17, 2022 08:49

bbatsov reviewed Jun 17, 2022

View reviewed changes

vmfhrmfoaj force-pushed the master branch from 12ec807 to 575e1b7 Compare June 17, 2022 09:28

vmfhrmfoaj added 3 commits June 17, 2022 18:35

Update CHANGELOG

fe60395

vmfhrmfoaj force-pushed the master branch from 575e1b7 to fe60395 Compare June 17, 2022 09:37

bbatsov merged commit 684a296 into alexander-yakushev:master Jun 17, 2022

This was referenced Jun 23, 2022

No completion candidates for desctucture namespaced keywords clojure-emacs/cider#3218

Closed

Some of the latest changes are missing in the released version #86

Closed

alexander-yakushev added the bug label Jun 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for namespaced keywords #83

Fix for namespaced keywords #83

vmfhrmfoaj commented Mar 26, 2022 •

edited

Loading

vemv left a comment

vemv Mar 26, 2022

vemv Mar 26, 2022

vemv Mar 26, 2022

vemv Mar 26, 2022

vmfhrmfoaj commented Mar 26, 2022

bbatsov commented Jun 17, 2022

vmfhrmfoaj commented Jun 17, 2022

vemv commented Jun 17, 2022

bbatsov commented Jun 17, 2022

bbatsov Jun 17, 2022

bbatsov commented Jun 17, 2022

	(def destructuring-key-names #{"keys" "strs" "syms"})
	(def destructuring-key-names (into #{} (map name) destructuring-keys))

		@@ -2,6 +2,9 @@

		### master (unreleased)

		- Complete local bindings declared in map destructuring with namespaced keywords.

Fix for namespaced keywords #83

Fix for namespaced keywords #83

Conversation

vmfhrmfoaj commented Mar 26, 2022 • edited Loading

vemv left a comment

Choose a reason for hiding this comment

vemv Mar 26, 2022

Choose a reason for hiding this comment

vemv Mar 26, 2022

Choose a reason for hiding this comment

vemv Mar 26, 2022

Choose a reason for hiding this comment

vemv Mar 26, 2022

Choose a reason for hiding this comment

vmfhrmfoaj commented Mar 26, 2022

bbatsov commented Jun 17, 2022

vmfhrmfoaj commented Jun 17, 2022

vemv commented Jun 17, 2022

bbatsov commented Jun 17, 2022

bbatsov Jun 17, 2022

Choose a reason for hiding this comment

bbatsov commented Jun 17, 2022

vmfhrmfoaj commented Mar 26, 2022 •

edited

Loading