View Definitions for non-virtual views

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

View Definitions for non-virtual views

Francisco Obispo

I created an RT ticket (I don’t know if it’s the right place), to document this behavior which I believe is a bug:

https://rt.cpan.org/Public/Bug/Display.html?id=122544

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.

regards


_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Thomas Sibley
I noticed this too, a while back, and wondered about it.  I believe the reason is that for folks who want to dump a database in order to start using DBIx::Class’s deployment tools, the authoritative view definition needs to start living in the .pm file.  For folks, like myself, who manage the database itself and use dbicdump just to sync the .pm files with it, the view’s underlying query is useless in the .pm.  I agree an option would be nice.

Thomas


On Jul 21, 2017, at 10:44 , Francisco Obispo <[hidden email]> wrote:

I created an RT ticket (I don’t know if it’s the right place), to document this behavior which I believe is a bug:

https://rt.cpan.org/Public/Bug/Display.html?id=122544

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.

regards

_______________________________________________
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@...


_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Matt S Trout-2
On Fri, Jul 21, 2017 at 10:51:15AM -0700, Thomas Sibley wrote:
> I noticed this too, a while back, and wondered about it.  I believe the reason is that for folks who want to dump a database in order to start using DBIx::Class’s deployment tools, the authoritative view definition needs to start living in the .pm file.  For folks, like myself, who manage the database itself and use dbicdump just to sync the .pm files with it, the view’s underlying query is useless in the .pm.  I agree an option would be nice.

It's kinda handy being able to see it in the files, too - the .pm files
are effectively documentation of the schema the code expects as well as
structural.

--
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Matt S Trout-2
In reply to this post by Francisco Obispo
On Fri, Jul 21, 2017 at 10:44:55AM -0700, Francisco Obispo wrote:
> I created an RT ticket (I don’t know if it’s the right place), to
> document this behavior which I believe is a bug:
>
> https://rt.cpan.org/Public/Bug/Display.html?id=122544

dbicdump is part of DBIx::Class::Schema::Loader not DBIx::Class itself
 
> 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?

--
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Thomas Sibley
In reply to this post by Matt S Trout-2
On Jul 21, 2017, at 12:32 , Matt S Trout <[hidden email]> wrote:
>
> On Fri, Jul 21, 2017 at 10:51:15AM -0700, Thomas Sibley wrote:
>> I noticed this too, a while back, and wondered about it.  I believe the reason is that for folks who want to dump a database in order to start using DBIx::Class’s deployment tools, the authoritative view definition needs to start living in the .pm file.  For folks, like myself, who manage the database itself and use dbicdump just to sync the .pm files with it, the view’s underlying query is useless in the .pm.  I agree an option would be nice.
>
> It's kinda handy being able to see it in the files, too - the .pm files
> are effectively documentation of the schema the code expects as well as
> structural.

Hmm, though the other parts of the generated file (columns, constraints, etc) take care of that too.  The full defining query of the view is, to me, more of the implementation details and semantics of the resulting schema.
_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Francisco Obispo
In reply to this post by Matt S Trout-2


On 21 Jul 2017, at 12:33, Matt S Trout wrote:

> On Fri, Jul 21, 2017 at 10:44:55AM -0700, Francisco Obispo wrote:
>> I created an RT ticket (I don’t know if it’s the right place), to
>> document this behavior which I believe is a bug:
>>
>> https://rt.cpan.org/Public/Bug/Display.html?id=122544
>
> dbicdump is part of DBIx::Class::Schema::Loader not DBIx::Class itself
>

Yes, but wasn’t sure were to post the request.


>> 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.

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.

I’m not saying it might not be useful for a lot of people, but for us
is not and I would like to have a switch to turn it off if the desire is
to leave it on by default, which again, I would advocate to do it the
other way around.

best regards,



> --
> 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@...

_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Karen Etheridge
If they were dumped as comments rather than strings, then there would be no runtime impact, but you still get the documentation benefit.  I'd certainly make use of such an option, as I sometimes perform a dump just to confirm that there were no surprising side effects to a change I made, and do not commit the output.

On Fri, Jul 21, 2017 at 3:14 PM, Francisco Obispo <[hidden email]> wrote:


On 21 Jul 2017, at 12:33, Matt S Trout wrote:

On Fri, Jul 21, 2017 at 10:44:55AM -0700, Francisco Obispo wrote:
I created an RT ticket (I don’t know if it’s the right place), to
document this behavior which I believe is a bug:

https://rt.cpan.org/Public/Bug/Display.html?id=122544

dbicdump is part of DBIx::Class::Schema::Loader not DBIx::Class itself


Yes, but wasn’t sure were to post the request.


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.

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.

I’m not saying it might not be useful for a lot of people, but for us is not and I would like to have a switch to turn it off if the desire is to leave it on by default, which again, I would advocate to do it the other way around.

best regards,




--
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@...

_______________________________________________
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@...


_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Matt S Trout-2
In reply to this post by Francisco Obispo
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Francisco Obispo

I’ve created a pull request for this feature:

https://github.com/dbsrgits/dbix-class-schema-loader/pull/13

It works by adding: -o omit_view_definitions=1 to the loader options.

I’ve tested it in my computer, and seems to work correctly,

best regards,

On 25 Jul 2017, at 12:24, Matt S Trout wrote:

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@...


_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Francisco Obispo

Any thoughts on the PR ?


On 26 Jul 2017, at 20:55, Francisco Obispo wrote:

I’ve created a pull request for this feature:

https://github.com/dbsrgits/dbix-class-schema-loader/pull/13

It works by adding: -o omit_view_definitions=1 to the loader options.

I’ve tested it in my computer, and seems to work correctly,

best regards,

On 25 Jul 2017, at 12:24, Matt S Trout wrote:

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@...

_______________________________________________
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@...


_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Thomas Sibley
On Aug 2, 2017, at 16:38 , Francisco Obispo <[hidden email]> wrote:
Any thoughts on the PR ?

Functionally it looks sane to me, by quick inspection.  A few review notes on style and process:

• At the very least, it should be reduced to the single real commit, instead of commit A, revert of A, commit B.
• The maintainers may require you to add tests for this new option.
• The maintainers would likely appreciate if you matched the surrounding code style, particularly with regard to indentation levels and brace hugging.

Cheers,
Thomas

_______________________________________________
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@...
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: View Definitions for non-virtual views

Francisco Obispo

I created a new PR, it has 2 commits, one to change the formatting and make it long-line style, which seems to be the dominant style in the code.

Who is the maintainer ?

On 2 Aug 2017, at 16:52, Thomas Sibley wrote:

Functionally it looks sane to me, by quick inspection.  A few review notes on style and process:

• At the very least, it should be reduced to the single real commit, instead of commit A, revert of A, commit B.

• The maintainers may require you to add tests for this new option.

• The maintainers would likely appreciate if you matched the surrounding code style, particularly with regard to indentation levels and brace hugging.

Cheers,

Thomas


_______________________________________________
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@...
Loading...