Closed GoogleCodeExporter closed 8 years ago
Hi,
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.
Original comment by alan.mcg...@gmail.com
on 18 Feb 2007 at 2:35
Hi,
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?
Original comment by olivier....@gmail.com
on 18 Feb 2007 at 4:08
I have done what I said on issue #5 and #6
Original comment by olivier....@gmail.com
on 21 Feb 2007 at 4:40
Patch rejected for reasons listed above. The new patches have been dealt with.
Closing this one.
Original comment by alan.mcg...@gmail.com
on 21 Feb 2007 at 7:57
Original issue reported on code.google.com by
olivier....@gmail.com
on 18 Feb 2007 at 12:20Attachments: