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 @kw_only macro #147

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

Add @kw_only macro #147

wants to merge 1 commit into from

Conversation

frankier
Copy link

@kw_only behaves the same as @with_kw but does not declare a default constructor when no inner constructor is found.

This is useful in occasions where you might like to declare a very generic outer constructor, as in the following example.

@kw_only
struct Example
    foo
    bar
end

function Example(args...)
    # figure out values of foo and bar based on args
    Example(; foo=foo, bar=bar)
end

With @with_kw, Example(42 "blah") would call the default inner constructor, whereas I would like for the outer constructor to be called in this case.

I can think it may be somewhat desirable in other situations where we want the keyword constructor to be the only interface, such as when there are a lot of fields in a struct and we might like to freely add fields anywhere without changing the constructor interface.

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #147 (de85ef3) into master (029dad2) will decrease coverage by 0.49%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   93.42%   92.92%   -0.50%     
==========================================
  Files           1        1              
  Lines         304      311       +7     
==========================================
+ Hits          284      289       +5     
- Misses         20       22       +2     
Impacted Files Coverage Δ
src/Parameters.jl 92.92% <84.61%> (-0.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@frankier
Copy link
Author

frankier commented Aug 25, 2022

(Better add a test... done)

`@kw_only` behaves the same as `@with_kw` but does not declare a default
constructor when no inner constructor is found.
@frankier
Copy link
Author

Please let me know if you have any concerns about merging this.

frankier added a commit to JuliaPsychometricsBazaar/ComputerAdaptiveTesting.jl that referenced this pull request Sep 14, 2022
@frankier
Copy link
Author

frankier commented Oct 4, 2022

Please let me know if there are any problems with this patch or if you would like to discuss the change.

@mauro3
Copy link
Owner

mauro3 commented Oct 4, 2022

Sorry, I'll get to it. Thanks for your work!

@frankier
Copy link
Author

frankier commented Oct 4, 2022

No need to apologize! Thanks for your work on the library and for taking a look at this patch.

@frankier
Copy link
Author

frankier commented Aug 11, 2023

Hi! It looks like there's a conflict now, but also I found a problem with the approach used in this PR.

An outer positional constructor appears to be a requirement when we want to have a type parameterised struct with a type parameter-free constructor (which necessarily includes keyword constructors). Since I want to hide positional constructors, I have figured out a new approach where we create a dummy type __Private (which can be made actually private when Julia supports this in 1.11) and dispatch on that in the first argument so that the positional constructor is only usable from the keyword constructor generated by Parameters.jl.

I realise the @kw_only use-case is might be a bit niche, and am currently using my own (possibly transitional) fork anyway, however, I can update this PR with the new approach + tests if this is something you would like to have in Parameters.jl. On the other hand if you consider this out-of-scope/too marginal, it's fine from my side if you want to reject this PR.

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.

2 participants