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

Merge raisesCondition() into raises() using EitherType. #127

Closed
wants to merge 6 commits into from

Conversation

player-03
Copy link
Contributor

I discussed making a macro for this, and while that would handle a wider variety of cases more reliably, Assert.hx is already way too long, and I couldn't bring myself to make it so much longer.

Instead, I found a way to get type T from the type argument while also allowing the user to pass Any type. EitherType attempts to unify the left type first, so it will match T if possible, and default to Any if not.

This only works when type comes before condition, and since both arguments are optional, it's possible the user will pass only the function, which will be incorrectly interpreted as being type, so we have to check for that. Arguably, the same can be said of msgNotThrown.

I still recommend changing type to Class<T> as soon as possible, but until then, this is the next best thing.

`EitherType` attempts to unify the left type first, so it will match `T` if possible, and default to `Any` if not. This only works when `type` comes before `condition`, and since both arguments are
optional, it's possible the user will pass only the function, which will be incorrectly interpreted as being `type`, so we have to check for that. Arguably, the same can be said of `msgNotThrown`.
Previously, Haxe typed it as `haxe.Exception`, so couldn't assign other values to it on static targets. Shadowing the variable is the simplest way around this.

And it should be declared above `checkCondition()`. Whoops.
@player-03
Copy link
Contributor Author

player-03 commented Dec 15, 2023

Whoops, forgot static targets exist for a moment there! But it should all work now.

Edit: nope, but it made progress at least. I'd like to reiterate that if we just changed type to Class<T>, everything would work so much better and it almost certainly wouldn't break any existing code.

@player-03
Copy link
Contributor Author

Ugh, it seems the trick with EitherType only works in Haxe 4.2 or later. In older versions, the compiler doesn't know e is a haxe.Exception, so it can't tell that e.message is supposed to be e.get_message(), and it returns null instead.

I see two viable options here:

  1. Change type to Class<T> (recommended).
  2. Don't support condition in older versions of Haxe.

Old versions of Haxe aren't good enough at type inference.
@player-03 player-03 changed the title Merge raisesCondition() into raises(). Merge raisesCondition() into raises() using EitherType. Dec 16, 2023
@RealyUniqueName
Copy link
Member

I think instead of trying to hack the type system it's better to keep both methods for now.
Since Assert.raises is widely used I'd like to keep it as a deprecated method but without @:deprecated meta for utest 2.0, and then add the meta in utest 2.1.
Rename Assert.raisesCondition to Assert.exception. It sounds like an appropriate naming.

@player-03
Copy link
Contributor Author

Assert.exception() sounds like a good name.

Is there a benefit to not adding the metadata immediately?

@player-03
Copy link
Contributor Author

player-03 commented Dec 20, 2023

I think instead of trying to hack the type system it's better to keep both methods for now.

Or do the solution I proposed in #128? That one doesn't "hack the type system" at all. I've explained why I think that's the better option, so maybe that ought to remain open and this should be closed.

Edit: or I can do a third pull request with the new name, and these two can both be closed.

@RealyUniqueName
Copy link
Member

RealyUniqueName commented Dec 20, 2023

Is there a benefit to not adding the metadata immediately?

Less distracting migration process for projects using utest.

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