-
Notifications
You must be signed in to change notification settings - Fork 164
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
ports/go: add more type conversions and tests #407
Conversation
We need to refine the tests first before we start refactoring. |
I have reverted your last PR changes: 1b8cd13 Do not modify the code if it does not add any value, for example linters etc.. or removing optional types, or just refactoring the ifs into switch, it changes the behavior because it removes the check of the casting if it was ok or not. You can add more type conversions, but remember, LONG is int64, not 32. I am also adding tests, for each code you add, please test it. You can add a folder called test with all the scripts for testing if you want, and verify each type you add that works. |
|
I am not gonna merge any other commit if you don't do TDD or you don't follow the instructions, neither Gil will merge it. It's just a way to prevent breaking things all the time, the project is big and there's not a lot of QA people for verifying it works properly, so TDD is super important. |
Also this commit for example, the one you have in the draft, it does not add any value, so this kind of commits won't be merged neither, you are free to add new type conversions but do not touch anything else if the refactor is not completely justified. |
629992e
to
05e49d7
Compare
The new code is pushed, please review. I see that a1a079f was able to run go_ports tests in CI(I'm not familiar with C toolchain), do I still need to add a new CI test for go_ports? Test result:
|
@iyear I am working on it, as soon as it is ready I will merge your changes and we will be able to check the result of the test in the CI (at least in linux). |
Description
Type of change
Checklist:
make test
orctest -VV -R <test-name>
)../docker-compose.sh test &> output
and attached the output.OPTION_BUILD_SANITIZER
or./docker-compose.sh test-sanitizer &> output
andOPTION_TEST_MEMORYCHECK
.OPTION_BUILD_THREAD_SANITIZER
or./docker-compose.sh test-thread-sanitizer &> output
.Helgrind
in case my code works with threading.make clang-format
in order to format my code and my code follows the style guidelines.If you are unclear about any of the above checks, have a look at our documentation here.