Skip to content
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

Metadata contains :sci.impl/op key #555

Closed
mk opened this issue Mar 31, 2021 · 9 comments
Closed

Metadata contains :sci.impl/op key #555

mk opened this issue Mar 31, 2021 · 9 comments

Comments

@mk
Copy link
Collaborator

mk commented Mar 31, 2021

version
master at e837435

problem
Extra :sci.impl/op key on metadata.

repro

(sci/eval-string "(meta ^:m {:foo {}})")
;;=> {:m true, :sci.impl/op :eval} 

Changed description after #555 (comment), original issue here for context:
version
0.2.4 and master at e837435

problem
Metadata isn't preserved correctly on e.g. nested maps.

repro

(sci/eval-string "(meta ^:m {:foo :bar})") ;; => {:m true}  
(sci/eval-string "(meta ^:m {:foo {}})") ;; => nil
@mk
Copy link
Collaborator Author

mk commented Mar 31, 2021

I've added a failing test to illustrate the problem in 01c734f. Feel free to cherry-pick it in case you want to look at a fix, I might get around to looking more into this later today or tomorrow.

@borkdude
Copy link
Collaborator

@mk I'm getting:

user=> (sci/eval-string "(meta ^:m {:foo {}})")
{:m true, :sci.impl/op :eval}

on master (the :sci.impl/op :eval should probably not be there).

@borkdude
Copy link
Collaborator

Is this on CLJS or CLJ?

@borkdude
Copy link
Collaborator

OK, can reproduce by adding the failing tests, to branch: issue-555-meta-nested-maps

@borkdude
Copy link
Collaborator

@mk Note that these tests do pass:

(testing "meta on nested maps"
    (is (:m (eval* "(meta ^:m {:foo :bar})")))
    (is (:m (eval* "(meta ^:m {:foo {}})"))))

so although there is an extra key, I can't reproduce your original issue.

@mk
Copy link
Collaborator Author

mk commented Mar 31, 2021

You're right, I'm just able to reproduce the behavior as above under 0.2.3 and 0.2.4 (like seen below) but not on master. I thought master was broken because of the test failure but it only failed because of the :sci.impl/op key like you've written above.

CleanShot 2021-03-31 at 15 46 18@2x

I've bisected and identified baf61ab as the commit that fixed it. So I think this can be closed as a duplicate of #546.

Do you want me to maybe create an extra issue for removing the :sci.impl/op key from metadata?

@borkdude borkdude changed the title Metadata not preserved correctly Metadata contains :sci.impl/op key Mar 31, 2021
@borkdude
Copy link
Collaborator

Let's just change the title of this issue... done. Feel free to update the original post.

@borkdude
Copy link
Collaborator

@mk Fixed!

@mk
Copy link
Collaborator Author

mk commented Mar 31, 2021

@borkdude thank you!

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

No branches or pull requests

2 participants