search

typst-community / valkyrie

Type safe type safety for Typst
Other
33 stars 4 forks source link

`z.assert.eq` and `.neq` are limited to `int` #49

Open TimeTravelPenguin opened 2 months ago

TimeTravelPenguin commented 2 months ago

Currently, the eq assertion looks like this:

/// Asserts that tested value is exactly equal to argument
#let eq(arg) = {
  assert-positive-type(arg, types: (int,), name: "Equality")

  return (
    condition: (self, it) => it == arg,
    message: (self, it) => "Must be exactly " + str(arg),
  )
}

Is the assertion here correct? I would like to assert on a string but this prevents that.

Why is this function (and the neq variant) limited to integers? Should this not be valid code for any equatable types? Would it not be more idiomatic to instead assert the type outside of this function?

I was looking to make a PR, but I am currently not sure how to go about this, in case there is a reason for it.

tingerrr commented 2 months ago

I don't know, to be honest, perhaps git blame can shed light on this.

Any rationale behind this or just an oversight, @JamesxX?

TimeTravelPenguin commented 2 months ago

I only noticed because I was messing around with an idea for a small personal package where I was perhaps abusing Valkyrie as a convenient validator of my objects.

If it is ok to change, then the str would need to change either to repr or to a helper method to choose between the two.

There are some concerns. For example, I am not sure how eq handles dict comparisons where entries are something like a function. It could simply panic, or there could be an optional argument to allow unsafe equality. Though, idk if that would be a good idea.

I will have a play with things if it's fine and propose a PR with my changes. I will add this to my list of things to do when my uni semester ends in Oct/Nov.

jamesrswift commented 2 months ago

I don't know, to be honest, perhaps git blame can shed light on this.

Any rationale behind this or just an oversight, @JamesxX?

Just an oversight I think. There's probably a reason but I can't see it

TimeTravelPenguin commented 2 months ago

Just an oversight I think. There's probably a reason but I can't see it

Thanks for getting back to us!

When I do get around to this, I will be sure to write up some tests. I'll be sure to shoot through any questions if I have any when I work on it.

Edit: If there is anything else that should be done in that PR, feel free to let me know.