On Tue, Oct 15, 2013 at 07:31:49PM +0200, Hrvoje Ribicic wrote: > > > > > > > > > > + > > > > Extra empty line. > > > > Fixed. > > > > > > > +-- | Collector type > > > +data CollectorType a b = > > > + CollectorSimple (Bool -> ConfigData -> [a] -> IO [(a, b)]) | > > > + CollectorFieldAware (Bool -> ConfigData -> [String] -> [a] -> IO [(a, > > b)]) > > > > Just to share my personal preference, I like to tie the constructor > > names to the type name. I especially like the way Template Haskell > > does it, for example, > > > > data Exp > > = ConE > > | LitE > > | VarE > > | ... > > > > As these types are not used often, it might be wiser for the type to be > the first thing the reader sees. > For more-often used types, I will follow this suggestion. > > > > Anyway, leave the choice of names up to you. In any case, > > indentation: > > > > data ... > > = ... > > | ... > > > > > Fixed. > > > > > + > > > -- * Helper functions > > > > > > -- | Builds an unknown field definition. > > > @@ -155,7 +161,7 @@ getRequestedJobIDs qfilter = > > > -- | Generic query implementation for resources that are backed by > > > -- some configuration objects. > > > genericQuery :: FieldMap a b -- ^ Field map > > > - -> (Bool -> ConfigData -> [a] -> IO [(a, b)]) -- ^ > > Collector > > > + -> CollectorType a b -- ^ Collector > > > > Saying that the 'FieldMap' is the 'Field map' and the 'CollectorType' > > is the 'Collector' isn't very helpful. > > > > Updated with more info. > > > > > > > -> (a -> String) -- ^ Object to name function > > > -> (ConfigData -> Container a) -- ^ Get all objects from > > config > > > -> (ConfigData -> String -> ErrorResult a) -- ^ Lookup > > object > > > @@ -181,7 +187,9 @@ genericQuery fieldsMap collector nameFn configFn > > getFn cfg > > > fobjects <- resultT $ filterM (\n -> evaluateFilter cfg Nothing n > > cfilter) > > > objects > > > -- here run the runtime data gathering... > > > > This comment is weird. Why not just 'Run the runtime data gathering' ? > > > > Wasn't my comment, old code. Fixed it though, and some other surrounding > comments. > > > Thanks for the review, interdiff: > > diff --git a/src/Ganeti/Query/Query.hs b/src/Ganeti/Query/Query.hs > index de0a711..32ce98e 100644 > --- a/src/Ganeti/Query/Query.hs > +++ b/src/Ganeti/Query/Query.hs > @@ -83,11 +83,10 @@ import Ganeti.Path > import Ganeti.Types > import Ganeti.Utils > > - > -- | Collector type > -data CollectorType a b = > - CollectorSimple (Bool -> ConfigData -> [a] -> IO [(a, b)]) | > - CollectorFieldAware (Bool -> ConfigData -> [String] -> [a] -> IO [(a, > b)]) > +data CollectorType a b > + = CollectorSimple (Bool -> ConfigData -> [a] -> IO [(a, b)]) > + | CollectorFieldAware (Bool -> ConfigData -> [String] -> [a] -> IO [(a, > b)]) > > -- * Helper functions > > @@ -176,8 +175,8 @@ getRequestedJobIDs qfilter = > -- The gathered data, or the failure to get it, is expressed through a > runtime > -- object. The type of a runtime object is determined by every query type > for > -- itself, and used exclusively by that query. > -genericQuery :: FieldMap a b -- ^ Field map > - -> CollectorType a b -- ^ Collector > +genericQuery :: FieldMap a b -- ^ Maps field names to field > definitions > + -> CollectorType a b -- ^ Collector of live data > -> (a -> String) -- ^ Object to name function > -> (ConfigData -> Container a) -- ^ Get all objects from > config > -> (ConfigData -> String -> ErrorResult a) -- ^ Lookup object > @@ -198,15 +197,15 @@ genericQuery fieldsMap collector nameFn configFn > getFn cfg > [] -> Ok . niceSortKey nameFn . > Map.elems . fromContainer $ configFn cfg > _ -> mapM (getFn cfg) wanted > - -- runs first pass of the filter, without a runtime context; this > - -- will limit the objects that we'll contact for exports > + -- Run the first pass of the filter, without a runtime context; this will > + -- limit the objects that we'll contact for exports > fobjects <- resultT $ filterM (\n -> evaluateFilter cfg Nothing n > cfilter) > objects > - -- here run the runtime data gathering... > + -- Gather the runtime data > runtimes <- case collector of > CollectorSimple collFn -> lift $ collFn live' cfg fobjects > CollectorFieldAware collFn -> lift $ collFn live' cfg fields fobjects > - -- ... then filter again the results, based on gathered runtime data > + -- ... then filter the results again, based on the gathered data
s/... then filter/Filter/ Rest LGTM Thanks, Jose > let fdata = map (\(obj, runtime) -> > map (execGetter cfg runtime obj) fgetters) > runtimes > > > > Hrvoje Ribicic > Ganeti Engineering > Google Germany GmbH > Dienerstr. 12, 80331, München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Steuernummer: 48/725/00206 > Umsatzsteueridentifikationsnummer: DE813741370 -- Jose Antonio Lopes Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
