Re: Object translation mechanism is, umm, broken. - Mailing list pgadmin-hackers

On Mon, 2011-06-13 at 10:18 +0100, Dave Page wrote:
> On Sat, Jun 11, 2011 at 1:37 PM, Guillaume Lelarge
> <guillaume@lelarge.info> wrote:
> > On Sat, 2011-06-11 at 11:22 +0200, Guillaume Lelarge wrote:
> >> On Fri, 2011-06-03 at 14:24 +0100, Dave Page wrote:
> >> > On Fri, Jun 3, 2011 at 1:45 PM, Guillaume Lelarge
> >> > <guillaume@lelarge.info> wrote:
> >> > >
> >> > > Hi,
> >> > >
> >> > > On Thu, 2011-06-02 at 08:54 +0000, Dave Page wrote:
> >> > > > [...]
> >> > > > This commit:
http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=4b8ae9d82c3b879eb02dda60908ed9590a674ec4
> >> > > > (particularly the code in pgObject.cpp) is causing problems. I keep
> >> > > > running into objects for which we now get messages like "Refreshing
> >> > > > unknown object of type Replication", which I believe used to work just
> >> > > > fine. I've just hit it with the Slony replication cluster object, and
> >> > > > now have a sneaking suspicion it's also going to affect all the other
> >> > > > slony object types.
> >> > > >
> >> > >
> >> > > Right.
> >> > >
> >> > > > Aside from the fact that the code is incomplete, it's a serious
> >> > > > modularity violation. We shouldn't have a giant conditional statement
> >> > > > in a parent class that has knowledge of each of the different classes
> >> > > > that might be derived from that object. Instead, each of the child
> >> > > > classes should override a common function.
> >> > > >
> >> > > > Can you look at this please?
> >> > > >
> >> > >
> >> > > Working on it right now.
> >> > >
> >> >
> >> >  Thanks - I appreciate it.
> >> >
> >>
> >> So I finally have something. Can you get a quick look at it to tell me
> >> if that's what you were looking for? I guess you'll be particularly
> >> interested in pgadmin/schema/pgObject.cpp.
> >>
> >> Thanks.
> >>
> >> BTW, still two issues on this (big) patch:
> >>  * Greenplum objects are taken care of, but I have no way to check them
> >>  * EDB objects still have no support. I can add that, but can't check
> >>    the code
> >>
> >
> > Same patch with EDB objects support (but still no tests on them). And
> > this time, compressed.
>
> That seems like a much cleaner design, though I must admit I'm
> surprised by the size of the patch.
>

Yeah, I was also surprised. I mostly had two different kinds of things
to do: add messages to fully unsupported objects (meaning something like
200 more lines in their respective files), and adding a
CreateCollection() function to partially supported objects (so,
declaration in .h, and code in .cpp). The sum of all these changes is
huge, but it's not a difficult code.

> I wonder if we should get it applied, and push out another beta ASAP?
>

I'm not against pushing out another beta. But I'm not sure we really
need it just for this patch. We seem to have some users that work on the
last git release. Let's see if they find something with this patch.


--
Guillaume
  http://blog.guillaume.lelarge.info
  http://www.dalibo.com