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 dict interface splitargdef and combineargdef #179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

willow-ahrens
Copy link
Contributor

This PR adds new methods splitargdef and combineargdef (in addition to existing splitarg and combinearg) that use dictionaries as an interface. The dict interface more completely expresses corner cases, as described in #178. In particular, it provides robust solutions for literal nothing argument defaults and avoids typeasserts when no type is given for an argument.

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #179 (341668a) into master (5e0fe08) will increase coverage by 1.13%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   74.68%   75.82%   +1.13%     
==========================================
  Files           9        9              
  Lines         403      426      +23     
==========================================
+ Hits          301      323      +22     
- Misses        102      103       +1     
Impacted Files Coverage Δ
src/utils.jl 70.15% <95.65%> (+3.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e0fe08...341668a. Read the comment docs.

@cstjean
Copy link
Collaborator

cstjean commented Jan 7, 2022

Hi Peter, thank you for the PR. I agree that the current situation is not ideal, but between the three options:

  1. Support two interfaces to splitarg
  2. Make a breaking release to replace splitarg with its Dict equivalent
  3. Leave splitarg as it is

I'm going to favour option 3, at least until there's another, more compelling reason to make a MacroTools 0.6 release. Having two interfaces for the same thing is more maintenance burden, and confusing for new users of the package. Furthermore, splitarg is only broken in a very marginal use case. It's unlikely that anyone will bump into it, and the work-around (use quote) sounds acceptable to me. It's a judgement call, obviously...

When 0.6 will be on the horizon, I would consider merging this PR with a deprecation from splitarg to splitargdef. Another name I find appealing is splitarg(Dict, ...) instead of splitargdef(...), but that's a minor detail I guess.

@willow-ahrens
Copy link
Contributor Author

willow-ahrens commented Jan 7, 2022

Sure thing! I think this would be great to include with the next version, and I wanted to file the PR before I forgot the details.

I chose splitargdef because it's clearer to deprecate a function for a differently named function (rather than a method for a method), and to match splitdef and splitstructdef, both of which simply use Dict.

@willow-ahrens
Copy link
Contributor Author

I will clarify that there is no current work-around for literal nothing input to splitarg, and no workaround to differentiate an Any typeassert from no typeassert based on the output of splitarg. These are still rare cases.

@cstjean
Copy link
Collaborator

cstjean commented Jan 7, 2022

I will clarify that there is no current work-around for literal nothing input to splitarg

There's a user-side work-around, of replacing f(arg=$x) with f(arg=$(QuoteNode(x))). Given that we're erroring in splitarg, it seems quite reasonable to me...

no workaround to differentiate an Any typeassert from no typeassert based on the output of splitarg

True.

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.

None yet

3 participants