On Thu, Aug 01, 2013 at 02:11:39PM +0200, Guido Trotter wrote:
> On Thu, Aug 01, 2013 at 12:34:44PM +0200, Jose A. Lopes wrote:
>
> > In a previous comment, you said to put it here.
> >
>
> Or before, if it doesn't break the current build.
>
> > >
> > > Are you gratuitously changing hook environment variable and opcode slot
> > > names?
> > > Why?
> >
> > Haskell and Python where inconsistent.
> > I had to fix this to make it consistent.
> >
>
> Then you have to keep python's version, which is the correct one
> (because it's in use today).
>
> > > Why do all these belong to the OpCodes module?
> > > I would say they just belong to a module which is part of the HsToPy
> > > module, or so.
> >
> > Fixed. Moved some of the code (what was possible) to src/hs2py.src.
> > Not everything can be moved because of Template Haskell restrictions.
> >
>
> Sure, but if it's specific to converting, perhaps a separate module that is
> under src/Ganeti/HS2PY would be best.
>
> > > Why the reordering?
> >
> > I had to check my generated code against the existing Python opcodes.py.
> > I had to reorder in order to make a diff.
> >
>
> Wouldn't it be better to make the diff with "sort" and/or to reorder the
> existing to minimize the patch diff? But ok, at this point let's leave it.
You are right! At that time I was not thinking about the impact the
code would have on the diff. I was using the Python as reference and,
therefore, reordered the Haskell code. But, in this case, I should've
reordered the Python code instead...
>
> > > > @@ -648,8 +1069,7 @@ opSummaryVal OpGroupEvacuate { opGroupName = s } =
> > > > Just (fromNonEmpty s)
> > > > opSummaryVal OpBackupPrepare { opInstanceName = s } = Just s
> > > > opSummaryVal OpBackupExport { opInstanceName = s } = Just s
> > > > opSummaryVal OpBackupRemove { opInstanceName = s } = Just s
> > > > -opSummaryVal OpTagsGet { opKind = k } =
> > > > - Just . fromMaybe "None" $ tagNameOf k
> > > > +opSummaryVal OpTagsGet { opKind = s } = Just (show s)
> > >
> > > What's changing here and why?
> >
> > TagObject is replaced by TagKind. Some code related to TagObject
> > (e.g., tagNameOf) also goes away.
> >
>
> Can this be done more explicitly? (in its own patch).
> That said, if it's compatible on the wire, ok.
>
> > > If UncheckedList was [JSValue] why did you decide to remove it?
> >
> > The code generation does not deal well with type aliasing. I have some
> > code to make it work with type aliasing but it created problems with
> > other types. I have to rething this when objects come into play. It
> > doesn't make sense to fix this right away because in order to
> > implement the objects this might need to be changed again. When I say
> > objects I mean both objects and RPCs.
> >
>
> Ack.
>
> Thanks,
>
> Guido
>
