OpenSWE1R / openswe1r

An Open-Source port of the 1999 Game "Star Wars Episode 1: Racer"
https://openswe1r.github.io/
GNU General Public License v2.0
313 stars 26 forks source link

Exit if exe is not found #90

Closed Nico01 closed 6 years ago

Nico01 commented 6 years ago

The program keep running even if not executable is found, this lead to a segfault.

JayFoxRox commented 6 years ago

Thanks for your PR. While the PR is fine as is, I have some criticism / constructive feedback:

This PR also raises some interesting questions about OpenSWE1R coding style:

I don't know why Travis was not ran for this PR. Travis received the PR notification, and tried to run the build 9 times, but all came back with "Nothing here" status. I'll manually try to trigger a build (on my fork) to see if this is macOS and Windows compatible. (Works fine: AppVeyor, Travis)


Please fixup / squash (and force push) your commits if you decide to change anything.

Nico01 commented 6 years ago

I suggest to use EXIT_SUCCESS/EXIT_FAILURE instead of 0/1 because improve readability and the behaviour is the same. As for error printing, check the return value and handle the result from the caller maybe better for keep the function cleaner. A coding style guideline would be nice and the code need to be formatted somehow (clang-format ?). inb4 my english suck

JayFoxRox commented 6 years ago

I've made some modifications to this PR:

I'll merge this PR after introducing a CLA (which you'll have to accept); and disclaimers to the README. More about this here.

Meanwhile, please review if my changes reflect your original intention.

clang-format is also planned for the future, it will be ran on Travis CI and report any style issues. I just don't have the time to work on these things right now: input, graphics, audio and multiplayer are more important. Also, your english is fine :)

JayFoxRox commented 6 years ago

@Nico01 the CLA has been finalized. Please sign it under this link.

(I'd appreciate if you sign it so we can test the ecosystem, but I'll still merge this anyway, simply because it's such a small change, so we are fine legally anyway)

Nico01 commented 6 years ago

Done :)