IT add: - some licence header - some class for model pipepline - fix some bug in pipepline - add with comment some opengl code for vertex/index buffer, effect - add some nunit test when it is possible but a lot of class can not been test because of sealed class with internal/private constructor
- duff patch 12.patch 86.73KB
Comment #1
Posted on Feb 18, 2007 by Helpful CatHi,
Firstly, that's a lot of code. Good stuff! There are a few problems though.
1) Your patch contains a lot of whitespace changes. I.e. changing four spaces into one tab. That's not good. It makes it really hard for me to see what exactly has changed.
2) You have a lot of unfinished things in your code. There are partial implementations everywhere. Those need to be clearly marked as such. One class contains a lot of commented out code for example. That can't really be committed.
3) There's just too much in that patch. Ideally there'd be one patch for each logical item. i.e. if you implemented the ModelMeshCollection class, it would be in its own patch as it is complete by itself. If you implemented a method which required that you touch 5 other files, then those 5 other files would be included in the diff.
4) That patch is from an old revision of the SVN. You would need to recreate your patch after updating your checkout. If i applied it as it is, it would undo some changes i made a few days ago.
So, in short. Smaller patches are better. You've done a lot of great work! And i'll commit stuff when it's been nunited (where possible) and comes in a nice small (ish) patch.
If you have any questions, give me a shout.
Thanks.
Comment #2
Posted on Feb 18, 2007 by Grumpy MonkeyHi,
thanks for feed back:
1) white space issue is because of, at start, someone said to me to switch tab to real tab in vs2005. SO I have done it. I will go back on this and make tab 4 spaces as default on vs2005
2) in fact, the model pipline is so big and I am not a pro of opengl. It is why when I see the size I do a patch to have help on it. I comment all my opengl code to be sure people check it before commit. How can I mark that a file is partialy implemented?
3) Ok I will try to cut my next patch in few small patch...
4)I have done a svn update before coding anything so you must have code something just after i done my path ;( My patch have been done few days ago. I have just commit it yesterday bu it is older.
Ok thanks for advices, I prefer critik than no answer ;) I will do my best to done few small patch and fix all space issue. For sealed class with internal/private constructor what can we do for nunit test?
Comment #3
Posted on Feb 21, 2007 by Grumpy MonkeyI have done what I said on issue #5 and #6
Comment #4
Posted on Feb 21, 2007 by Helpful CatPatch rejected for reasons listed above. The new patches have been dealt with. Closing this one.
Status: PatchRejected
Labels:
Type-Patch
Priority-Medium
Component-Logic