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.
_______________________________________________
List: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/dbix-class
IRC: irc.perl.org#dbix-class
SVN: http://dev.catalyst.perl.org/repos/bast/DBIx-Class/
Searchable Archive:
http://www.grokbase.com/group/dbix-class@lists.scsys.co.uk
