On Fri, Nov 13, 2015 at 10:32 AM, Klaus Aehlig <[email protected]> wrote:
> > Optional suggestions present, else LGTM. > > > > @@ -216,8 +218,9 @@ getOnlineNodes = filter (not . nodeOffline) . > F.toList > > > . configNodes > > > -- | Returns the default cluster link. > > > getDefaultNicLink :: ConfigData -> String > > > getDefaultNicLink = > > > - nicpLink . (M.! C.ppDefault) . fromContainer . > > > - clusterNicparams . configCluster > > > + let ppDefault = UTF8.fromString C.ppDefault > > > + in nicpLink . (M.! ppDefault) . fromContainer > > > + . clusterNicparams . configCluster > > > > > > > Since this pattern repeats itself: would it make sense to make the > constant > > a ByteString and modify our Python-generating code to handle this > > adequately? > > I considered it, but I think it would change infra structure too much, > especially > for achange that we wouldn't benefit from at run time. We can think about > extending > the python code generation, but if we do so, it definitely should be a > separate > patch. > > If you think it is really important, I can, of course change it; but my > preference is > to not change it. > > I understand and agree. Should we add more code using this pattern, we can revisit this decision. > > > Replace key array creation with map lookup? > > That would be the following interdiff. I'm a bit worried it might change > the test > itself too much, but if you think it's fine, I'll include it. > > diff --git a/test/hs/Test/Ganeti/Query/Network.hs > b/test/hs/Test/Ganeti/Query/Network.hs > index 6e01825..aee0376 100644 > --- a/test/hs/Test/Ganeti/Query/Network.hs > +++ b/test/hs/Test/Ganeti/Query/Network.hs > @@ -69,9 +69,9 @@ prop_getGroupConnection group = > -- yields 'Nothing'. > prop_getGroupConnection_notFound :: NodeGroup -> String -> Property > prop_getGroupConnection_notFound group uuid = > - let net_keys = map UTF8.toString . Map.keys . fromContainer . > groupNetworks > - $ group > - in notElem uuid net_keys ==> isNothing (getGroupConnection uuid group) > + let net_map = fromContainer . groupNetworks $ group > + in not (UTF8.fromString uuid `Map.member` net_map) > + ==> isNothing (getGroupConnection uuid group) > > Well, it's a bit cleaner, so I would prefer that version in. LGTM > -- > Klaus Aehlig > Google Germany GmbH, Dienerstr. 12, 80331 Muenchen > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschaeftsfuehrer: Matthew Scott Sucherman, Paul Terence Manicle > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.
