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

Fix range check for ChuckSomeDice() #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bbrk24
Copy link

@bbrk24 bbrk24 commented Apr 2, 2022

Fixes #41

It turns out the error wasn't about floats per se -- ChuckSomeDice(0, 0) would cause the same panic, while ChuckSomeDice(0, 1.5) wouldn't.

@vercel
Copy link

vercel bot commented Apr 2, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/zackradisic/aussieplusplus/5PXUsgyiMpB5WS3Jc5EiyYuJLdq4
✅ Preview: https://aussieplusplus-git-fork-bbrk24-range-check-fix-zackradisic.vercel.app

@bbrk24
Copy link
Author

bbrk24 commented Apr 2, 2022

Hm, the preview still has the error. I'm not quite sure what's going on.

@bbrk24 bbrk24 marked this pull request as draft April 2, 2022 00:32
@bbrk24
Copy link
Author

bbrk24 commented Apr 2, 2022

What's weird is it works locally on my machine. I'm completely new to Rust, so I have no idea how to diagnose the discrepancy.

@bbrk24 bbrk24 marked this pull request as ready for review April 2, 2022 06:59
@bbrk24
Copy link
Author

bbrk24 commented Apr 2, 2022

Yeah I won’t be able to figure out the difference. I’m hopeful it’s just something wrong with the preview.

@dspingarn
Copy link

dspingarn commented May 16, 2022

I'm guessing that the preview is still running the master branch somehow, but only @zackradisic would know for sure, since I can't see the vercel configuration...

I was able to run this change on my local machine and can see that changing from > to >= works correctly in making sure the proper error message is printed if the range is 0,0!
...I deployed your change on my own instance of vercel here -> https://aussieplusplus-4y3o5ei6a-dspingarn.vercel.app/

@dspingarn
Copy link

dspingarn commented May 16, 2022

actually, I know what the issue is, @bbrk24
You need to run make wasm to make sure the live demo that's generated contains your new code change.
Right now, your logic change is just contained in the aussie++ code, so the preview won't reflect that

@bbrk24
Copy link
Author

bbrk24 commented May 16, 2022

You need to run make wasm to make sure the live demo that's generated contains your new code change

Huh. I figured such a step would be done automatically or wouldn't touch checked-in files. I tried it just now, locally, and I got this linker error:

'emcc.bat' is not recognized as an internal or external command, operable program or batch file.

I'm not entirely sure why a cmd error came up when I'm using bash, and the solutions I'm finding online don't seem to work either.

@dspingarn
Copy link

emcc is the webassembly compiler
...this project definitely needs a better "first time setup" guide
anyway. You can follow https://emscripten.org/docs/getting_started/downloads.html to figure out how to configure the emsdk on your machine

@dspingarn
Copy link

dspingarn commented May 17, 2022

from there, when you run make wasm, if it's successful, you should see some files in the site folder get updated.
cd into the site folder, and that's a node project of some kind... you can install and run it using yarn (which, incidentally, I believe the yarn build task is using next.js in the background, but that's not important)
you can do yarn build and then yarn run start to have it running on localhost:3000

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.

thread 'main' panicked at 'cannot sample empty range'
2 participants