Code review comment for lp://staging/~tomdroid-dev/tomdroid/sync-ui

Revision history for this message
Olivier Bilodeau (plaxx) wrote :

First, I only glanced quickly at your work but I'm very excited to see your work!

Second, sorry if anything here was already mentionned elsewhere but I haven't replied..

Third, all of this is a matter of discussion. Given the right arguments I will change my mind.

1) order of things
I'm a bit confused with the merge hiearchy here..

Is this based on web-sync meaning we need to merge web-sync first or does this integrates web-sync so merging this will result in a web-sync + sync-ui integration?

2) commons-codec
What is commons-codec used for? Was there no built-in alternative already available? I'm trying to avoid adding a 64k jar into the tomdroid package here.

3) signpost
Not sure what you did with signpost.. haven't looked but can't you use a released jar (with a version we can track bugs against, etc.) instead of having a git checkout of their stuff? Did you need to modify their upstream code? If you can't proceed with released code, can you get rid of the git metadata (signpost/.git/) and identify in a README.tomdroid what revision/repo is this code from and what custom changes you needed done. Also, I would like the eclipse metadata removed.

4) file headers
Some new files are missing the usual project header. Be careful to assign copyright to the right person who created the work. If you are willing to give me the copyright feel free to do so. In any case you will be attributed for your work elsewhere.

Start looking at these please. Once dealt with I'll checkout the branch and play with it.

Thanks!

« Back to merge proposal