I'd feel bad if this issue hadn't come up a few times in the past. Sending in large patches that do multiple things is silly. Even worse is that these are fundamental changes to where things are located but we haven't even decided there is a problem with how things are now or even whether things are better after these changes.
I looked the patch over, you move include/user.php to include/classes/user.php but you didn't just move the class, you moved the class AND functions that call in to the user class. Why bother separating things into functions and classes if this isn't even the case in the patch?
I wish we could have at least talked about this patch before you went through the effort to make these changes. I can't see any reason why we should separate functions and classes. Right now the name of the php file indicates what the contents of the file are. Lets talk about where we are going with this and look at some examples of how these changes would improve things before we decide to make them.
Chris
On Saturday 18 December 2004 3:16 pm, Jonathan Ernst wrote:
Hello,
I certainly agree that I could have separated apidb->appdb changes from the rest (but replacing it is trivial and is functionnaly equivalent), but I can't seriously make one patch for each file moved as it'll require to patch every dependent files for each move.
The files in / import files in /include if I move one file from /include to /include/classes I have to patch all files.
Then I have to wait until the patch is commited which will take one day because of timezones.
Then I have to move the second file and re-change every single file that includes it. and so on at least 25 times (because there are 25 moved files) without counting the code moving between includes.
I will _try_ to split the patch in three or four smaller patches and I hope we can find a way to make it in as it would allow for more interesting (and smaller ;-) ) changes later.
As I'm not an expert, if you have advices on how to split it more efficiently I'm all open as you know.
See you,
<gack> you _can't_ be serious.
- One patch to do one thing. For example each of the following should
their own patch.
Change apidb_header() and apidb_footer() to appdb_header() and appdb_footer() Change apidb_fullurl() to appdb_fullurl Move include/tableve.php to include/classes/tableve.php Move include/application.php to include/classes/application.php ...
I know you probably will end up sending more than a dozen patches that way. However, I can't approve of a wholesale change, like this, in one go.
</gack>
The changes you propose are good ones. Just break them down.
--
Tony Lambregts