search

ruma / js_int

JavaScript-interoperable integer types for Rust
MIT License
16 stars 8 forks source link

Fix for jplatte/js_int#17 #20

Closed Noble-Mushtak closed 3 years ago

Noble-Mushtak commented 3 years ago

I saw this project in This Week in Rust and wanted to contribute. I implemented the two TryFrom implementations by extending the convert_impls macro to have three new arguments, $ot8, $ot16, $ot32. I used "o" to stand for the word "opposite" as in oppositely-signed. I also added a test to int.rs to test the TryFrom<Int> for uN implementations and a test to uint.rs to test the TryFrom<UInt> for iN implementations.

One question I had is, do you want implementations for TryFrom<Int> for u64 and TryFrom<Int> for u128? You said N < 64 for both bullet points, but I do not see a reason why implementing TryFrom<Int> for u64 and u128 would not work.

Anyway, please let me know if I implemented this in a non-idiomatic/incorrect way. I am very new to Rust and would appreciate any feedback.

edit(jplatte): Fixes #17

Noble-Mushtak commented 3 years ago

I just realized all of the status checks seem to have failed. When I click "Details," in the setup log, it says "Build failed," but when I run cargo build on my local machine, it seems to work fine. I'm pretty new to this and don't know too much about CI checks, so I might be misunderstanding. If anyone has guidance on how to fix these checks, I'd definitely appreciate that.

DevinR528 commented 3 years ago

It seems to be a problem with the build script

+ cd js_int
.tasks/test: line 4: cd: js_int: No such file or directory

I'm not sure exactly the environment that builds.sh sets up so this may be related to something else not exactly sure.

jplatte commented 3 years ago

Hey, thanks for the PR! I must have been affected by some GitHub bug because I never got notified about this. I think I know what's wrong with the CI, though I'm not sure right now how to fix it. I will look into it.

jplatte commented 3 years ago

Have to do this tomorrow 😞

For the record, the issue is that builds.sr.ht works without specifying sources in the build manifest(s) when pushing things to git.sr.ht, but for the GitHub integration you need to specify that. I have to find out how to specify sources without breaking CI outside of PRs...

jplatte commented 3 years ago

Sorry for letting this sit for so long. Just fixed the merge conflicts. It looks like formatting is not right (needs cargo fmt) and there's also a missing import in the tests (make sure to run cargo check test locally, not just cargo build).

jplatte commented 3 years ago

CI is still failing because of the missing import.

Noble-Mushtak commented 3 years ago

OK, I think CI is passing now. Is there anything else I should fix before this PR can be merged?

jplatte commented 3 years ago

No, merged. 🙂