LGTM on the second version, Thanks,
Guido On Wed, Mar 7, 2012 at 10:11 PM, Iustin Pop <[email protected]> wrote: > On Wed, Mar 07, 2012 at 05:44:31PM +0000, Guido Trotter wrote: >> LGTM >> >> Thanks, >> >> Guido >> >> (maybe abstracting the generation of the error message would help >> making it more readable?) > > Possibly. What do you think about: > > > diff --git a/htools/Ganeti/HTools/Cluster.hs b/htools/Ganeti/HTools/Cluster.hs > index 1f868f5..e9e423a 100644 > --- a/htools/Ganeti/HTools/Cluster.hs > +++ b/htools/Ganeti/HTools/Cluster.hs > @@ -1003,8 +1003,9 @@ evacOneNodeInner :: Node.List -- ^ Cluster node > list > -> EvacInnerState -- ^ New best solution > evacOneNodeInner nl inst gdx op_fn accu ndx = > case applyMove nl inst (op_fn ndx) of > - OpFail fm -> either (const $ Left $ "Node " ++ Container.nameOf nl ndx ++ > - " failed: " ++ show fm) (const accu) accu > + OpFail fm -> let fail_msg = "Node " ++ Container.nameOf nl ndx ++ > + " failed: " ++ show fm > + in either (const $ Left fail_msg) (const accu) accu > OpGood (nl', inst', _, _) -> > let nodes = Container.elems nl' > -- The fromJust below is ugly (it can fail nastily), but > > I'm more worried if the construct "either (const …) (const x) x" is > readable, as opposed to: > > case x of > Left _ -> … > Right _ -> x > > thanks, > iustin > >> On Tue, Mar 6, 2012 at 11:59 PM, Iustin Pop <[email protected]> wrote: >> > Instead of manually case-ing on the Either contents, let's just use >> > either with const functions (not really readable, but…). >> > --- >> > htools/Ganeti/HTools/Cluster.hs | 7 ++----- >> > 1 files changed, 2 insertions(+), 5 deletions(-) >> > >> > diff --git a/htools/Ganeti/HTools/Cluster.hs >> > b/htools/Ganeti/HTools/Cluster.hs >> > index 11b7cae..a8acd2c 100644 >> > --- a/htools/Ganeti/HTools/Cluster.hs >> > +++ b/htools/Ganeti/HTools/Cluster.hs >> > @@ -999,11 +999,8 @@ evacOneNodeInner :: Node.List -- ^ Cluster >> > node list >> > -> EvacInnerState -- ^ New best solution >> > evacOneNodeInner nl inst gdx op_fn accu ndx = >> > case applyMove nl inst (op_fn ndx) of >> > - OpFail fm -> >> > - case accu of >> > - Right _ -> accu >> > - Left _ -> Left $ "Node " ++ Container.nameOf nl ndx ++ >> > - " failed: " ++ show fm >> > + OpFail fm -> either (const $ Left $ "Node " ++ Container.nameOf nl >> > ndx ++ >> > + " failed: " ++ show fm) (const accu) accu >> > OpGood (nl', inst', _, _) -> >> > let nodes = Container.elems nl' >> > -- The fromJust below is ugly (it can fail nastily), but >> > -- >> > 1.7.9.1 -- Guido Trotter Google - Corporate Computing Services SRE Google Ireland Ltd. : Registered in Ireland with company number 368047. Gordon House, Barrow Street, Dublin 4, Ireland.
