Add support for namespaced maps #17
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vemv
reviewed
Sep 8, 2023
parseclj already added support for namespaced maps in 2018 (in commit b40670a56147214f0486763529897cb688a09692). As Alex Miller said in edn-format/edn#78 > Clojure introduced namespace map syntax in Clojure 1.9. The Clojure > edn reader was also updated to support the non-autoresolved parts of > namespace map syntax (edn doesn't do anything autoresolved like > ::foo or #::foo{}). > > The edn spec should be updated to a new version that includes the > namespace map syntax such as #:foo{:bar 1} (syntax alternative for > {:foo/bar 1}). So it's pretty clear that the intention is for edn spec to support namespaced maps (the reference implementation in Clojure already does!). In addition to the use case mentioned in issue clojure-emacs#16, not supporting this seems to cause trouble in CIDER when integrating with shadow-cljs (clojure-emacs/cider#3437) Fixes: clojure-emacs#16
parseedn-read never sees the full value of the `:ast` keys as defined in `parseedn-test-data.el`. In fact, it doesn't know about it at all. The only functions that see anything related to the `:ast` values are `parseedn-reduce-leaf` and `parseedn-reduce-branch`. But they are called back from `parseclj` when needed, to "reduce" (transform the current AST node definition into the associated Emacs Lisp data type). As commented in clojure-emacs#17 (comment), they very likely are remnants of when parseedn and parseclj were the same library.
vemv
reviewed
Sep 16, 2023
parseedn.el
Outdated
| @@ -113,10 +113,24 @@ on available options." | |||
| ((eq :lbracket token-type) (apply #'vector children)) | |||
| ((eq :set token-type) (list 'edn-set children)) | |||
| ((eq :lbrace token-type) (let* ((kvs (seq-partition children 2)) | |||
| (hash-map (make-hash-table :test 'equal :size (length kvs)))) | |||
| (hash-map (make-hash-table :test 'equal :size (length kvs))) | |||
There was a problem hiding this comment.
Comparing the 'before' and 'after' of defun parseedn-reduce-branch, I sense that now it mixes levels of abstraction.
Before it looked more concise and higher-level.
The simplest fix would be to extract most of the logic to a separate defun. Although possibly this might mean something else (e.g. this could be fixed in parseclj?) - analysis much welcome.
Before adding the code to support prefixed maps, the branch reduction function looked more concise and higher-level. After adding the support it seemed like it mixed levels of abstraction. Extracting the nitty-gritty details of how to build the maps (especially in the prefixed map case) should help in keeping the levels of abstraction separated.
vemv
pushed a commit
that referenced
this pull request
Sep 29, 2023
parseedn-read never sees the full value of the `:ast` keys as defined in `parseedn-test-data.el`. In fact, it doesn't know about it at all. The only functions that see anything related to the `:ast` values are `parseedn-reduce-leaf` and `parseedn-reduce-branch`. But they are called back from `parseclj` when needed, to "reduce" (transform the current AST node definition into the associated Emacs Lisp data type). As commented in #17 (comment), they very likely are remnants of when parseedn and parseclj were the same library.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
parseclj already added support for namespaced maps in 2018 (in commit b40670a56147214f0486763529897cb688a09692).
As Alex Miller said in edn-format/edn#78
So it's pretty clear that the intention is for edn spec to support namespaced maps (the reference implementation in Clojure already does!).
In addition to the use case mentioned in issue #16, not supporting this seems to cause trouble in CIDER when integrating with shadow-cljs (clojure-emacs/cider#3437
Fixes: #16