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

Add clojure style destucturing #1940

Merged
merged 2 commits into from
Jan 31, 2021
Merged

Conversation

peaceamongworlds
Copy link
Contributor

@peaceamongworlds peaceamongworlds commented Jan 16, 2021

Closes #1936.

Builds on #1328.

Mainly updated old syntax and refactored some of the code, but the main ideas are essentially the same.

Also added defn+, fn+, and let+ macros, which have clojure-style destructuring within their arguments.

@Kodiologist
Copy link
Member

main and defmain shouldn't be necessary in a test file. pytest should be able to find your tests and run them.

@peaceamongworlds
Copy link
Contributor Author

I think all the tests issues are resolved now.

@Kodiologist
Copy link
Member

Thanks for your work on this. It looks like it's coming along.

It looks to me like the syntax for dictionary destructuring is still backwards. For example, to get the number 3 from {"a" {"b" {"c" 3}}}, you write (=: {{{x "c"} "b"} "a"} …). Wouldn't it make more sense the other way around?

It's not clear that fn+ (or defn+) works properly with &optional, &rest, etc. You should probably test that.

Test how :keys works with mangling.

Test what happens when the destructuring pattern fails to match the rvalue, as in (:= [x] []).

Describe what ifp does in a docstring or comment.

"This might not be necessary in the future, see #846" — This issue is closed.

In the tests, there are places where you use =: where setv would suffice, like the definition of D.

The subroutine vals@ is only used once, so I would delete it and just write (lfor k "abcd" (get D k)) in place of (vals@ D "abcd").

In the assertion (assert (= (, a b (tuple the-rest)) (, "a" "b" (, "c" "d" "e" "f" "g")))), I think it would be better to test the type of the-rest rather than convert it to a tuple. It should be a string in this case, I think.

Don't forget to update the copyright year.

Remove all TODO, FIXME, and similar comments. Plans for Hy should go in the issue list, not the code.

@peaceamongworlds
Copy link
Contributor Author

It looks to me like the syntax for dictionary destructuring is still backwards. For example, to get the number 3 from {"a" {"b" {"c" 3}}}, you write (=: {{{x "c"} "b"} "a"} …). Wouldn't it make more sense the other way around?

I think that this is a matter of convention. Since I was trying to copy the way clojure does destructuring, it makes sense to me to copy this as well.

It's not clear that fn+ (or defn+) works properly with &optional, &rest, etc. You should probably test that.

Test how :keys works with mangling.

Added a test for this. It seems to just work.

(=: {:keys [a? -b? c->d]} {:a? 1 :-b? 2 :c->d 3})
[a? -b? c->d] ; => [1 2 3]

Test what happens when the destructuring pattern fails to match the rvalue, as in (:= [x] []).

Sorry I should have added more documentation. All the 'patterns' return None if they don't find the variable, so in this case x would be None. However if the shape of your 'pattern' doesn't match the shape of the data, then you'll get an error.

Describe what ifp does in a docstring or comment.
"This might not be necessary in the future, see #846" — This issue is closed.
The subroutine vals@ is only used once, so I would delete it and just write (lfor k "abcd" (get D k)) in place of (vals@ D "abcd").
Don't forget to update the copyright year.
Remove all TODO, FIXME, and similar comments. Plans for Hy should go in the issue list, not the code.

Done

In the assertion (assert (= (, a b (tuple the-rest)) (, "a" "b" (, "c" "d" "e" "f" "g")))), I think it would be better to test the type of
the-rest rather than convert it to a tuple. It should be a string in this case, I think.

I made both list destructuring and iterator destructuring convert their arguments into a list and iterator respectively, so the rest argument will be a list or an iterator, not a string. I think that this makes more sense this way as it allows iterable to be passed to both. So something like (=: [a :& rest] (filter even? (range 10))) will fail because filter objects aren't subscriptable.

It's not clear that fn+ (or defn+) works properly with &optional, &rest, etc. You should probably test that.

I added keyword destructuring to lists, which allows the :& argument in lists to be destructured as either a list or a dictionary. So something like this should work:

(defn+ foo [[a b] {:keys [c d]} :& {:strs [e f]}]
    "bar foo"
    [a b c d e f])

(foo [1 2] {:c 3 :d 4} "e" 5 "f" 6) ; => [1 2 3 4 5 6]

Thus you don't really need &rest etc. If you do add them, they won't be themselves recursively destructable like the other arguments, hence I thought this made more sense.

@Kodiologist
Copy link
Member

Good work!

In the tests, there are places where you use =: where setv would suffice, like the definition of D.

I think you forgot this one.

Test strs with mangling, too. It looks like (=: {:strs [hello?]} {"is_hello" 1}) fails to match hello?, whereas (=: {:strs [hello?]} {"hello?" 1}) works; is this intentional?

While it may be true that the special symbols like &optional aren't necessary to write functions with fn+, users may still try to use them, perhaps by mistake, so test what should happen. For example, does (fn+ [&optional [a b]] …) unpack a tuple, use b as the default value of a, raise an error, or something else?

@peaceamongworlds
Copy link
Contributor Author

In the tests, there are places where you use =: where setv would suffice, like the definition of D.

I think you forgot this one.

Fixed

Test strs with mangling, too. It looks like (=: {:strs [hello?]} {"is_hello" 1}) fails to match hello?, whereas (=: {:strs [hello?]} {"hello?" 1}) works; is this intentional?

I didn't think of this, so currently, there the macro doesn't do any explicit mangling. What should be the desirable behaviour? Unmangle everything?

While it may be true that the special symbols like &optional aren't necessary to write functions with fn+, users may still try to use them, perhaps by mistake, so test what should happen. For example, does (fn+ [&optional [a b]] …) unpack a tuple, use b as the default value of a, raise an error, or something else?

Currently all the special symbols are just treated like any other variable name. So (fn+ [&optional [a b]] ...) expects 2 variables, and it will attempt to destructure the second one. Should I explicitly disallow their use, or just specify in the documentation that they aren't special?

@Kodiologist
Copy link
Member

I didn't think of this, so currently, there the macro doesn't do any explicit mangling. What should be the desirable behaviour? Unmangle everything?

Hmm. It's not obvious. Since the introduction of "late mangling" in #1517, the symbol hello? really does stringify to "hello?"; mangling happens only at compile-time and only for certain symbols. I'd say let's keep this behavior for now, with a test that makes it explicit, and if we have to change it later, we can.

Should I explicitly disallow their use, or just specify in the documentation that they aren't special?

I think the latter makes sense. And you can add a test that shows that all the would-be special symbols are treated as ordinary in this context.

@peaceamongworlds
Copy link
Contributor Author

I didn't think of this, so currently, there the macro doesn't do any explicit mangling. What should be the desirable behaviour? Unmangle everything?

Hmm. It's not obvious. Since the introduction of "late mangling" in #1517, the symbol hello? really does stringify to "hello?"; mangling happens only at compile-time and only for certain symbols. I'd say let's keep this behavior for now, with a test that makes it explicit, and if we have to change it later, we can.

Should I explicitly disallow their use, or just specify in the documentation that they aren't special?

I think the latter makes sense. And you can add a test that shows that all the would-be special symbols are treated as ordinary in this context.

Done

@Kodiologist
Copy link
Member

Okay, looking good. Here's what's left:

  • Write a documentation page for the exported macros. Since we don't currently have a way to synchronize docstrings with the manual, remove the docstrings. (You can still have docstrings for internal stuff that isn't in the manual, like ifp.)
  • Add an item to NEWS.
  • Squash your changes. (This PR doesn't make any changes to old code; it just adds new code; so I don't think we need to preserve the process.)
  • In a separate commit, add yourself to AUTHORS.

@peaceamongworlds
Copy link
Contributor Author

I decided to rename =: to setv+. That seems more consistent with the other macro names, and I just like it more than =:.

@peaceamongworlds
Copy link
Contributor Author

Okay, looking good. Here's what's left:

* Write a documentation page for the exported macros. Since we don't currently have a way to synchronize docstrings with the manual, remove the docstrings. (You can still have docstrings for internal stuff that isn't in the manual, like `ifp`.)

* Add an item to NEWS.

* Squash your changes. (This PR doesn't make any changes to old code; it just adds new code; so I don't think we need to preserve the process.)

* In a separate commit, add yourself to AUTHORS.

I think that's all done now.

@Kodiologist
Copy link
Member

I think you need to add a link to your new page from index.rst for it to be navigated to. Otherwise, looks good. Thanks for your contributions, and welcome aboard!

@peaceamongworlds
Copy link
Contributor Author

I think you need to add a link to your new page from index.rst for it to be navigated to. Otherwise, looks good. Thanks for your contributions, and welcome aboard!

Thanks! I've added it.

@allison-casey
Copy link
Contributor

This makes me unseemly happy! Thank you peaceamongworlds!

@allison-casey
Copy link
Contributor

By the way, should we also have defn/a+ and fn/a+ variants?

@Kodiologist
Copy link
Member

If you like.

@peaceamongworlds
Copy link
Contributor Author

By the way, should we also have defn/a+ and fn/a+ variants?

I've added them now.

@allison-casey
Copy link
Contributor

allison-casey commented Jan 23, 2021

It doesn't look like the async variants actually expand to use the defn/a and fn/a forms

@peaceamongworlds
Copy link
Contributor Author

It doesn't look like the async variants actually expand to use the defn/a and fn/a forms

That was silly of me. It's fixed now.

@allison-casey
Copy link
Contributor

You dropped this 👑

@gilch
Copy link
Member

gilch commented Feb 16, 2021

This also closes #1326.

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

Successfully merging this pull request may close these issues.

Destructuring function arguments
4 participants