search

danwent / Perspectives-Server

network notary implementation for the Perspectives project
http://perspectives-project.org
GNU General Public License v3.0
50 stars 13 forks source link

Minor bugfix: argument -p ineffective #15

Closed mwgamera closed 11 years ago

mwgamera commented 11 years ago

The --webport option was ignored and the only way to change port from default was to use environment.

daveschaefer commented 11 years ago

Hey, great catch! Thanks for the pull request :)

Fixing this is important but I think perhaps we should only use args.webport if that argument was supplied. What do you think about something like #16? The tests work on my local machine but let me know if you find a test case that doesn't work.

If that patch looks okay to you I may pull my patch but I still want to give you the credit. Let me know how you would like to be credited (or if you would prefer not to be listed). My email is on my profile if you prefer to discuss that offline.

Thanks again!

mwgamera commented 11 years ago

I'm not well versed in python libraries (I didn't know argparse can validate integers itself) so I just made it work the way I thought it was originally intended to.

Your patch looks good but I'd recommend making it behave consistently. Originally it would simply throw on int(·) when used with --envport but with $PORT unset or invalid so I assumed it is expected to work the same way for --webport. Your comment on #16 says that it “checks for both being valid integers” (probably a good idea) and it indeed does for --webport option but it simply throws when $PORT is set to something that doesn't parse as integer and --envport is used. Furthermore I think that falling back to default value when $PORT is unset while --envport option is explicitly given is a bad idea. I'd treat all situations where program is told to take something from the environment but it's not set there as errors, but this is just my opinion.

As for the credit, you've already credited me in commit message, no need to exaggerate.

daveschaefer commented 11 years ago

falling back to default value when $PORT is unset while --envport option is explicitly given is a bad idea

Good point - I changed it to throw ValueError. It should now throw exceptions if anything is wrong and use the numbers if they are correct. Look okay?

daveschaefer commented 11 years ago

Hey @mwgamera - I have merged #16 so I will close this. Thanks for the help! :) Let me know if you spot any other bugs.