-
Notifications
You must be signed in to change notification settings - Fork 9
Normalise optics when constructing VarName; remove extra constructors #123
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 82.43% 83.56% +1.12%
==========================================
Files 2 2
Lines 296 292 -4
==========================================
Hits 244 244
+ Misses 52 48 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 16098270412Details
💛 - Coveralls |
b70c160
to
1ffc9ad
Compare
498fc38
to
de2eccb
Compare
AbstractPPL.jl documentation for PR #123 is available at: |
de2eccb
to
390ccd6
Compare
390ccd6
to
7bf3162
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
maybe add a comment somewhere saying that ComposedOptics
is just a alias of ComposedFunctions
? (I opted for ComposedOptics
when migrating to Accessors because I thought it's simpler to understand in context of the migration. But I support use ComposedFunction
now.)
That makes sense. I guess it's really Julia that had me confused 😄 , since Accessors writes julia> using Accessors
julia> @o _.a.b
(@o _.a.b)
julia> dump(ans)
(@o _.a.b) (function of type ComposedFunction{PropertyLens{:b}, PropertyLens{:a}})
outer: PropertyLens{:b} (@o _.b)
inner: PropertyLens{:a} (@o _.a) I'll add a note! |
Closes #122.
I also added a changelog, because I've actually found it super helpful for DynamicPPL/Turing. I'm going to copy paste from it to describe this PR:
VarName(vn, optic)
(this wasn't deprecated, but is IMO quite dangerous as it silently discards the existing optic invn
), andVarName(vn, ::Tuple)
(which was deprecated).Also changed all usage of
ComposedOptic
toComposedFunction
(it's the same thing: https://github.com/JuliaObjects/Accessors.jl/blob/18e9ddbc727ed4e48a3c19ffa00a02f7f407cdb5/src/optics.jl#L129).