pfalcon / pycopy-lib

Standard library of the Pycopy project, minimalist and light-weight Python language implementation
https://github.com/pfalcon/pycopy
Other
246 stars 70 forks source link

random: fix randrange to accept 1 as parameter #39

Closed crazyiop closed 5 years ago

crazyiop commented 5 years ago

makes randrange(1) return 0 instead of raising a ValueError. This indirectly fix the following with correct behavior:

pfalcon commented 5 years ago

This is cool, but ideally, I'd like to see tests for this stuff (which would behave the same as for CPython). There's already test_randrange.py, so new testcases might go into it (though I have to admit that the existing test there is rather adhoc and may behave differently with CPython). If you go for it, addition of a test should be a separate commit.

crazyiop commented 5 years ago

Ok, no problem. I'll do that when I get the time to dig enough about tests to follow this repo guidelines as best as possible.

pfalcon commented 5 years ago

I now hit this case myself. But my solution is different: https://github.com/pfalcon/pycopy-lib/commit/664f50e35c21a4ef9a042d34d87a9b351479fa0d, with commit message describing the motivation.

I'll do that when I get the time to dig enough about tests to follow this repo guidelines as best as possible.

It's actually as simple as it could (hopefully): a test should be named following pattern test_*.py, and you should run it as pycopy test_foo.py. If it doesn't throw exception, it passes. It's so simple there's not even a centralized test runner for entire pycopy-lib, that's still on TODO, you have to run testcases manually on a case by case basis like above.

So, a test for this case is https://github.com/pfalcon/pycopy-lib/commit/1d45880515a1f8368bc1ae3d65740db31904fc93 . I wonder if it would pass for this patch...

crazyiop commented 5 years ago

Sorry I had no time for that... I see that you already merged a fix and the tests. Thanks for your work! :+1:

pfalcon commented 5 years ago

I assume you reviewed it and don't have concerns. Thanks for the report, closing then.