On 31 December 2014 at 20:41, Dennis E. Hamilton <[email protected]> wrote:
> -- replying below to -- > From: jan i [mailto:[email protected]] > Sent: Wednesday, December 31, 2014 09:55 > To: [email protected]; Dennis Hamilton > Subject: Re: platform with minzip, commit or not commit. > > On 31 December 2014 at 18:46, Dennis E. Hamilton <[email protected]> > wrote: > > > -- replying below to -- > > From: jan i [mailto:[email protected]] > > Sent: Wednesday, December 31, 2014 08:28 > > To: [email protected] > > Subject: platform with minzip, commit or not commit. > > > > Hi All. > > > > I am in serious doubt. I have completed the isolation of minzip with a > > platform api in wrapper.c > > > > BUT we have no test cases for DFZipFile.c, so I cannot really be sure I > > have done it all correct. > > > > <orcmid> > > I assume that you haven't broken the build. > > So the question is, what is the policy on making untested changes? > > > > ESPECIALLY when a new API is being introduced. > > > +1, this is really something we need to form a policy about.....I am scared > about what a stray commit can do. > > > > > > I would do this in two stages. > > > > 1. Do not depend on the wrapper yet, and do not build with it. (If > > this creates CMake problems, keep the wrapper on a branch. This > > might also help stay out of the way of changes that Kelly is making.) > > > > Commit the wrapper so you are not stuck with uncommitted changes when > > you want to sync with the repository. > > > > Allow the wrapper to be reviewed and any tests for it to be devised > > before modifying other code to use the wrapper. > > > I cannot follow you here. I cannot commit the changes without the wrapper, > then it will not build, secondly we have no place where I can make > wrapper.c available apart from the development branch. > > <orcmid> > I don't understand why you have to commit this on the development > branch. > I would have thought that you'd create a new repository branch > specifically for introducing this and having it reviewed and tested. > Any changes to CMakeFile.txt would be on the branch too. > Sometimes I simply think to complicated....I was somehow fixed on that we only have 2 remote branches, I have created a temporary branch for the review. > > (I have the sneaking suspicion that I will make myself crazy dealing > with CMake.) > Welcome in the club, it was one of the first discussions I had with peter, but he convinced me of the benefits. I still believe we should have visual studio solution checked in, so that windows developers are up an running in no time. > > > > > 2. When cutover to using the wrapper API happens, do that without > other > > changes. Since the direct use of minizip worked, any failures can > > be isolated to introduction of the Wrapper API. This will be a lot > > easier to identify and handle. > > > That is exactly what I have prepared now. > > Wrapper.c + DFZipFile.c (the only consumer) and the needed CMakeFile.txt > changes. > > Errors can be in either DFZipFile.c or wrapper.c. Since I moved code from > one into the other I need to commit both at the same time. > > But I listen carefully to the arguments, because this is a really important > matter for the future. Of course I believe it works, but so will any > developer in the future, and we are all just humans. > > thanks for commenting. > > <orcmid> > And have you come to a conclusion? > </orcmid> > yes you will see a request for review in a moment. rgds jan I. > rgds > jan i. > > > > </orcmid> > > > > I am on my way writing test cases for platform in general, but that will > > take another week, and I would like to commit the minizip changes, since > it > > involved a number of CMakeFiles.txt. Be aware the platform test cases > will > > not ensure that DFZipFile.c works. > > > > What is the general opinion, should I run the risk and push to master > > (hoping you all will help catch the problems, and not put me in the > > dungeon) or should I wait until someone provides DFZipFile.c test cases > > (which should normally be the case). > > > > Once I have completed test cases for platform, it is my intention to > > continue with core, but start with a long discussion. > > > > rgds > > jan i > > > > > >
