Skip to content

Conversation

@icweaver
Copy link
Member

Partially addresses #59

Experimenting with replacing Parameters.@with_kw with Base.@kwdef

@icweaver icweaver mentioned this pull request Feb 21, 2025
@codecov
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f2a860d) to head (3580b40).
Report is 8 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master       #61      +/-   ##
===========================================
+ Coverage   99.62%   100.00%   +0.37%     
===========================================
  Files          10        10              
  Lines         267       296      +29     
===========================================
+ Hits          266       296      +30     
+ Misses          1         0       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@icweaver icweaver marked this pull request as ready for review April 7, 2025 18:05
@icweaver icweaver requested a review from cgarling April 7, 2025 18:05
@icweaver
Copy link
Member Author

icweaver commented Apr 7, 2025

This should be ready for review too now. Thanks!

@icweaver icweaver changed the title Trying out Base.@kwdef Replace Parameters.@with_kw with Base.@kwdef Apr 7, 2025
@cgarling
Copy link
Member

cgarling commented May 8, 2025

Seems fine to me, main point is to get rid of Parameters dependency? I do not use @kwdef in my own code so I don't know anything about the differences between the two implementations.

@icweaver
Copy link
Member Author

icweaver commented May 8, 2025

Sgtm! Right, the aim is to drop the Parameters dependency

Same. The only difference I am aware of is that Parameters supports @asserts, which one of our laws depended on:

@assert x0 0 "`x0` must be ≥ 0, got $x0"
@assert gamma 0 "`gamma` must be ≥ 0, got $gamma"

There seems to be some discussion about moving away from @assert these days though, so I don't feel too bad about not using it in this PR

@cgarling
Copy link
Member

cgarling commented May 8, 2025

Looks good to merge

@icweaver icweaver merged commit d46b630 into JuliaAstro:master May 8, 2025
10 checks passed
@icweaver icweaver deleted the kwdef branch May 8, 2025 21:15
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