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 constraint support #119

Merged
merged 17 commits into from
Aug 29, 2024
Merged

Add constraint support #119

merged 17 commits into from
Aug 29, 2024

Conversation

omus
Copy link
Member

@omus omus commented Jun 11, 2024

Adds basic constraint support for Legolas schemas so you can define constraints on the relationships between fields. Previously, this could be worked around by adding such logic into a field:

julia> using Legolas: @schema, @version

julia> @schema "old.constraint" OldConstraint

julia> @version OldConstraintV1 begin
           a
           b = begin
               @assert a == b
               b
           end
       end

julia> OldConstraintV1(; a=1, b=2)
ERROR: ArgumentError: Invalid value set for field `b` (2)
Stacktrace:
 [1] OldConstraintV1(; a::Int64, b::Int64)
   @ Main ~/.julia/dev/Legolas/src/schemas.jl:545
 [2] top-level scope
   @ REPL[4]:1

caused by: AssertionError: a == b
Stacktrace:
 [1] macro expansion
   @ ./REPL[3]:4 [inlined]
 [2] OldConstraintV1(; a::Int64, b::Int64)
   @ Main ~/.julia/dev/Legolas/src/schemas.jl:580
 [3] top-level scope
   @ REPL[4]:1

Unfortunately, the old approach would produce an extra ArgumentError which could create confusion. Additionally, when utilizing more complicated constructor logic for b these kinds of constraints can be less clear than declaring them separately like so:

julia> using Legolas: @schema, @version

julia> @schema "new.constraint" NewConstraint

julia> @version NewConstraintV1 begin
           a
           b
           @check a == b
       end

julia> NewConstraintV1(; a=1, b=2)
ERROR: Legolas.CheckConstraintError: a == b
Stacktrace:
 [1] macro expansion
   @ ./REPL[3]:4 [inlined]
 [2] NewConstraintV1(; a::Int64, b::Int64)
   @ Main ~/.julia/dev/Legolas/src/schemas.jl:619
 [3] top-level scope
   @ REPL[4]:1

Closes #69

@omus omus requested a review from jrevels as a code owner June 11, 2024 20:25
@omus
Copy link
Member Author

omus commented Jun 11, 2024

GitHub actions is currently having an outage

@jrevels
Copy link
Member

jrevels commented Jun 12, 2024

xref #69

I'd like to use @check for this instead of @assert, and have it throw a Legolas-defined CheckConstraintError as laid out in the xref'd sketch

want to avoid teaching users that it's literally the same as Julia's built-in@assert, as we might add more sugar/behaviors later to this (e.g. #20)

@omus
Copy link
Member Author

omus commented Jun 12, 2024

I'd like to use @check for this instead of @assert; want to avoid teaching users that it's literally the same as Julia's built-in@assert, as we might add more sugar/behaviors later to this (e.g. #20)

Good call. I started off with @assert just to prove this was a viable approach. I will update this PR to use @check

Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally good with this. I think a bit of explaining about the line number node manipulations would be helpful for future maintainers.

How reliable is the line-number manipulation across julia versions? I have a vague sense that it might uh not be the most reliable thing but I have nothing to back that up...

test/runtests.jl Outdated Show resolved Hide resolved
src/schemas.jl Show resolved Hide resolved
@omus
Copy link
Member Author

omus commented Aug 28, 2024

How reliable is the line-number manipulation across julia versions? I have a vague sense that it might uh not be the most reliable thing but I have nothing to back that up...

There's a test for it and I've found it to be quite reliable so far

@omus
Copy link
Member Author

omus commented Aug 28, 2024

I'll be waiting until #123 is merged before merging this.

@omus
Copy link
Member Author

omus commented Aug 28, 2024

#123 has been merged. I need to update the version here and finish some downstream testing I started.

@omus omus merged commit 7a50019 into main Aug 29, 2024
4 of 6 checks passed
@omus omus deleted the cv/constraints branch August 29, 2024 18:17
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.

@check convenience syntax for record/field-level value constraints
3 participants