[Dbix-class] View Definitions for non-virtual views

[Matt S Trout]

On Fri, Jul 21, 2017 at 03:14:33PM -0700, Francisco Obispo wrote:
> Yes, but wasn’t sure were to post the request.

The Schema-Loader queue would've been ok, here is probably best anyway.
 
> >>The result is that `.pm`’s derived from a view from `dbicdump` are
> >>larger than they need to be, inherently using more memory, and with
> >>no real good purpose. There should be an option to turn this
> >>behavior off.
> >
> >While I don't see any reason we wouldn't take a patch to supply
> >such an
> >option, I confess to being surprised that this was noticeable -
> >how did
> >you measure the memory usage with and without, and how big was the
> >difference?
> >
> 
> The diffs in git started getting very big all of the sudden, and in
> the company that I work, that started raising questions on the QA
> team, plus we have some areas where we have fairly large postgres
> views whose complexity is completely abstracted in a simple
> structure that has no interest to the ORM.

Personally, I quite like being able to see when the meaning of one of
my result classes just changed, but I can absolutely see "this is
beyond our abstraction boundary and therefore we don't want it in the
diff" as an argument.
 
> Those fairly large views, force us to store the definition in RAM
> (as a perl string) now just for the sake of documentation, but in
> reality, at least in our system, is the wrong place to store the
> view definition. I don’t want to have to explain a more junior
> developer that the real view lives in SQL, and what lives in the .PM
> is just a snapshot of how it looked when the schema was dumped.

Talking about RAM usage - which I suspect is negligible - is an incredibly
weak argument here and if you don't have numbers I'd suggest just not making
that argument.

"This is outside our abstraction boundary" is a good argument.

"This is making the diffs when we update the schema ugly" is a good argument.

"This might confuse junior developers" is a good argument.

All of those three are IMO each sufficient to justify a patch.

So, what I'd suggest is, that you look at putting together a patch that

(a) allows to toggle whether the SQL should be in code, docs, or nowhere
(b) emits a comment immediately above where view_definition normally is
    that mentions it's reflected from the schema and mentions the option

That way junior developers using existing systems shouldn't be confused
anymore because the comment will tell them, and the existence of the comment
should also mean anybody with a use case like yours will find the option.

Plus it can also emit a comment when it *doesn't* reflect the view
information, so if somebody later wants to switch to DBIC-controlled
deployment they'll also find the option to tweak.

I don't think the default should change, though, because I know of quite a
few projects that rely on Schema::Loader'ed schemas being deployable to set
up test databases and similar, and if anybody with needs like yours can easily
find the option anyway, it seems impolite to break their setups.

Does that all make sense? Fancy having a go at a patch?

-- 
Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/   http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN
commercial support, training and consultancy packages could help your team.