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

feat: implement times(Double) and div(Double) #60

Closed

Conversation

sargunv
Copy link

@sargunv sargunv commented Jan 27, 2025

Fixes #53

@sargunv sargunv marked this pull request as ready for review February 2, 2025 09:31
@sargunv
Copy link
Author

sargunv commented Feb 2, 2025

I've updated the docs, tests, and SaturatingLong impl according to the review comments!

@kevincianfarini
Copy link
Owner

Almost there. Are you interested in also contributing SaturatingLong.div(Double)?

@sargunv sargunv changed the title feat: implement times(Double) feat: implement times(Double) and div(Double) Feb 3, 2025
@sargunv
Copy link
Author

sargunv commented Feb 3, 2025

I've made the updates, and added the div implementation and tests as well. I had an "AI" assistant replicate the div tests and implementation based on times, so do double check the tests are correct (they look right to me)

Comment on lines +333 to +343
fun dividing_by_double_zero_throws() {
assertFailsWith<Exception> { // ArithmeticException on most platforms; Exception on JS
10L.saturated / 0.0
}
assertFailsWith<IllegalArgumentException> {
POSITIVE_INFINITY / 0.0
}
assertFailsWith<IllegalArgumentException> {
NEGATIVE_INFINITY / 0.0
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like this test somehow isn't working for wasmJS. Also -- if the exception types differ per platform we could use assertFails instead of assertFailsWith.

Copy link
Owner

Choose a reason for hiding this comment

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

   RuntimeError: divide by zero
      at <alchemist_test>.kotlin.Long__div-impl (wasm://wasm/<alchemist_test>-004f0842:wasm-function[2259]:0x3521d)
      at <alchemist_test>.io.github.kevincianfarini.alchemist.internal.SaturatingLong__div-impl (wasm://wasm/<alchemist_test>-004f0842:wasm-function[3351]:0x45efe)
      at <alchemist_test>.io.github.kevincianfarini.alchemist.internal.SaturatingLong__div-impl (wasm://wasm/<alchemist_test>-004f0842:wasm-function[3352]:0x45f12)
      at <alchemist_test>.io.github.kevincianfarini.alchemist.internal.SaturatingLong__div-impl (wasm://wasm/<alchemist_test>-004f0842:wasm-function[3354]:0x45f40)
      at <alchemist_test>.io.github.kevincianfarini.alchemist.SaturatingLongTest.dividing_by_double_zero_throws (wasm://wasm/<alchemist_test>-004f0842:wasm-function[4931]:0x67af5)
      at <alchemist_test>.io.github.kevincianfarini.alchemist.test fun$io.github.kevincianfarini.alchemist test fun$SaturatingLongTest test fun$dividing_by_double_zero_throws test fun.invoke (wasm://wasm/<alchemist_test>-004f0842:wasm-function[4651]:0x5b25e)
      at <alchemist_test>.kotlin.test.TeamcityAdapter$test$lambda.invoke (wasm://wasm/<alchemist_test>-004f0842:wasm-function[4226]:0x573d6)
      at <alchemist_test>.kotlin.test.TeamcityAdapterWithPromiseSupport.runOrScheduleNextWithResult (wasm://wasm/<alchemist_test>-004f0842:wasm-function[4108]:0x558e5)
      at <alchemist_test>.kotlin.test.TeamcityAdapterWithPromiseSupport.runOrScheduleNextWithResult (wasm://wasm/<alchemist_test>-004f0842:wasm-function[4109]:0x55923)
      at <alchemist_test>.kotlin.test.TeamcityAdapter.test (wasm://wasm/<alchemist_test>-004f0842:wasm-function[4249]:0x58f4c)

Copy link
Owner

Choose a reason for hiding this comment

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

I'm happy to get this over the finish line if you want @sargunv. Let me know!

Copy link
Author

Choose a reason for hiding this comment

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

Go for it! If not, I'll get to it during the weekend

@kevincianfarini
Copy link
Owner

I unfortunately had some weird issues trying to push directly to your branch, so I opened another PR with all of your changes and some of the remaining tweaks.

#65

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.

Allow multiplying by Double, not just Int
2 participants