← Back to team overview

widelands-dev team mailing list archive

Re: [Merge] lp:~widelands-dev/widelands/net-uuid into lp:widelands

 

Review: Needs Fixing

Reviewing now. Sorry that this waited so long for me. 

1) third_party/crypto is under "public domain" which is not a license in most parts of the world. I expect issues with debian about this. Can we ask the author to relicense under Apache 2 or MIT? Thes licenses are mostly what the author intended with public domain, but more formal. We can merge this before that happens of course.

2) I am not a fan of 'send_uuid' as a variable name - its semantics are unclear. I do not understand if it means: 'client expects server to send a uuid' or 'server expects client to send a uuid'. Maybe that becomes clearer while I read on, but a better name would be appreciated.

3) internet_gaming_protocol.h: remove all mentions of the nounce implementation? e.g. you write that the UUID replaces the nounce. But the nounce was only an interim version of the metaserver that we no longer support - so why keep this historic backage in the documentation?

I added some consts, and a few clarifying comments. After addressing these changes this lgtm.

Will review the go code next.
-- 
https://code.launchpad.net/~widelands-dev/widelands/net-uuid/+merge/332264
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-uuid.


References