Various fixes and cleanups #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The following plugin adds the META.json file. This is considered the more modern META data format.
The following plugin will add the provides metadata to the distribution, which is considered a best practice.
croak() is used in some places but not everywhere, so might as well report from the caller everywhere.
In Dancer2-Plugin-DBIC, DBIx::Class::Schema::Loader is mentioned as a requirement. It's a lazy optional one. It's also hard to spot because it's being loaded as a string using Module::Load. Instead, this makes it a bit clearer by using 'require' directly. It would be better to make this an optional requirement instead of a default one.
Currently DBIx::Class::Schema::Loader is loaded optionally, but reported as a hard dependency. (See previous commit.) This makes it recommended, but optional.
ironcamel
reviewed
Feb 10, 2017
lib/DBICx/Sugar.pm
Outdated
| die "You must provide a schema_class option or install $dbic_loader." | ||
| if $@; | ||
| $dbic_loader->naming( $options->{schema_loader_naming} || 'v7' ); | ||
| eval { require DBIx::Class::Schema::Loader; 1; } |
There was a problem hiding this comment.
Why did you change this from eval { load $dbic_loader };?
There was a problem hiding this comment.
The eval is a bit clearer and I always prefer using barewords for require where possible.
I changed it because I hoped it will be clearer for others to read. This was missed in a comment on Dancer2-Plugin-DBIC in which recognizing this was used was missed.
Feel free to skip this if you don't like it.
There was a problem hiding this comment.
I think the old code is cleaner. Please change this part back. Also, please make sure all lines are <= 80 characters long.
|
On Sat, 11 Feb 2017, Sawyer X wrote:
> @@ -95,23 +96,21 @@ sub schema {
$schema = $schema_class->connect( @conn_info );
}
} else {
- my $dbic_loader = 'DBIx::Class::Schema::Loader';
- eval { load $dbic_loader };
- die "You must provide a schema_class option or install $dbic_loader."
- if $@;
- $dbic_loader->naming( $options->{schema_loader_naming} || 'v7' );
+ eval { require DBIx::Class::Schema::Loader; 1; }
The `eval` is a bit clearer and I always prefer using barewords for `require` where possible.
You also change "load" to "require".
I changed it because I hoped it will be clearer for others to read. This was missed in a comment on Dancer2-Plugin-DBIC in which recognizing this was used was missed.
If you refer to my comment, I dit not mis it.
Niether Dancer2-Plugin-DBIC nor DBICx-Sugar has a
"use DBIx::Class::Schema::Loader;" statement.
So, DBIx::Class::Schema::Loader is not a requirement,
just an option. Prevent the dependency and just die.
|
|
Aha. Okay. |
This reverts commit 4f80a6f.
|
I reverted that specific commit. I had made no other changes that pass 80 characters. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[This falls under my Pull Request Challenge, extending my PR for Dancer2-Plugin-DBIC into this distribution.]
This includes similar fixes as those submitted to Dancer2-Plugin-DBIC. You can view this as "Step 2 out of 4".
The fixes and improvements, in summary, include:
DBIx::Class::Schema::Loaderand declare it appropriately, resolving #18 in Dancer2-Plugin-DBIC.diecalls withcroak.