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

sigmatch: support optional params with last block arg(s) #18631

Merged
merged 10 commits into from
Aug 20, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Aug 2, 2021

this PR solves a major pain point of optional params with last block arg(s)

fix nim-lang/RFCs#405
fix #884
fix #14346

template fn(a = 1, b = 2, body) = discard
fn(1, 2): # already works
  bar
fn(a = 1): # now works
  bar

ditto with multiple block args via do:

template fn(a = 1, b = 2, body1, body2) = discard
fn(a = 1): # now works
  bar1
do:
  bar2

etc, see tests/misc/trfc405.nim

@c-blake
Copy link
Contributor

c-blake commented Aug 2, 2021

Maybe one of your tests should check the >1 body passed case with a do:?

@timotheecour timotheecour force-pushed the pr_sigmatch_optional_block branch from a1461ab to 536a447 Compare August 2, 2021 20:04
@timotheecour timotheecour changed the title sigmatch: support optional params with last block arg sigmatch: support optional params with last block arg(s) Aug 2, 2021
@timotheecour
Copy link
Member Author

Maybe one of your tests should check the >1 body passed case with a do:?

i was planning on supporting that in future work, but i've now done it, see tests

@timotheecour timotheecour marked this pull request as ready for review August 2, 2021 22:51
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Aug 2, 2021
@disruptek
Copy link
Contributor

Why would I want this? To make statement lists more ambiguous?

@disruptek
Copy link
Contributor

I think I simply don't understand your example. Can you explain why you added bar to the block?

@disruptek
Copy link
Contributor

For that matter, can you explain why you add when true:? This is noise and I am tired of reading it.

@timotheecour
Copy link
Member Author

Why would I want this

because optional params are useful and using overloads instead causes known problems, see linked issues

@timotheecour timotheecour requested a review from Araq August 11, 2021 10:48
@Araq
Copy link
Member

Araq commented Aug 11, 2021

The implementation matches the RFC, the only minor gripe would be that you should extract the new search loop into a helper proc. Will comment on the RFC.

@timotheecour timotheecour force-pushed the pr_sigmatch_optional_block branch from 645af28 to c85ad28 Compare August 11, 2021 17:40
@timotheecour
Copy link
Member Author

you should extract the new search loop into a helper proc. Will comment on the RFC.

done, PTAL

@timotheecour
Copy link
Member Author

ping @Araq , this is a big usability improvement; (and I've replied in nim-lang/RFCs#405 (comment) to your comment in RFC)

@Araq Araq merged commit f291007 into nim-lang:devel Aug 20, 2021
@timotheecour timotheecour deleted the pr_sigmatch_optional_block branch August 20, 2021 17:49
@Clyybber
Copy link
Contributor

Clyybber commented Oct 19, 2021

This caused a regression reported on the forums: https://forum.nim-lang.org/t/8513
EDIT: Also reported on github #19015

@solo989
Copy link
Contributor

solo989 commented Oct 21, 2021

This could work properly with varargs if the matching algorithm matched the first non default argument that hasn't already been matched instead of working backwards like it does now.

That way it wouldn't try to fill the varargs argument in the case where it should fill other non default arguments first.

PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
)

* sigmatch: support optional params with last block arg
* add tests
* works with multiple block args
* cleanup
* address comment
@metagn metagn mentioned this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
6 participants