[Patch] Fix planner notes gas change output logic

Dirk Hohndel dirk at hohndel.org
Fri Jun 19 05:42:18 PDT 2015


On Fri, Jun 19, 2015 at 10:12:06AM +0200, Robert Helling wrote:
> > 
> > The attached patch should do just that.  Please have a look.
> 
> Thanks a lot for this. Too bad I currently have virtually no time to look at this in any detail and due to a family wedding this weekend possibly not before Monday.

I know that feeling.

> Still a few remarks:
> 
> 1) Could you split your patch into 2-3 patches, one for the segment type, another one for the new logic? git add -p is very helpful in doing this (as I recently learned).

Yes. Small patches are good. Smaller patches are better.
You will often see me do one liners were the commit message is five times
longer than the actual change. This makes a huge difference when reviewing
patches.

And git makes it very easy to then reorder things, rearrange things, clean
up older commits, etc.

git add -p     allows you to pick exactly what you want in a patch. Even
               the 'e' option where you can hand edit the patch is easy
git rebase -i  makes it easy to go back and work on the series of patches
               so they make sense, are in the right order and don't
	       contain redundant pieces

> 2) Don’t use the gamix.gas.permille members directly or you will run into problems since air is stored as 0%O2+0%2He. If you want to treat the special cases separately, you are likely doublicating code. Please always use the helper functions get_o2(&gasmix) and get_he(&gasmix) if you really want the content and to test if two gases are the same, there is gasmix_distance(&gasmix1, &gasmix2).

Completely agree.

> 3) I must say, I am not really a fan of the 0min stops. But people might convince me. What I had intended to do is to change the plan() such that after doing a gas change, it always waits for a minute (and that minute might later be configurable by the user). I think this is realistic as doing a gas change with all checks while continuously ascending is just unrealistic.

Even though I earlier commented about the gas changes while ascending, I
am perfectly happy to have us force a one minute stop for a gas change.

/D


More information about the subsurface mailing list