add min_app_githooks_version config option #46

Merged
merged 1 commit into from Mar 19, 2017

Conversation

Projects
None yet
3 participants
@lharey
Contributor

lharey commented Mar 15, 2017

Refs #28

As per our email conversation re PR request challenge. I hope this is what you were after.

Lisa

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 15, 2017

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 6560e44 on lharey:master into 1399c6d on guillaumeaubert:master.

coveralls commented Mar 15, 2017

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 6560e44 on lharey:master into 1399c6d on guillaumeaubert:master.

@guillaumeaubert

Thank you for this PR, @lharey! Just a couple of notes below, but this works well and it will be very useful.

+ name => 'commit-msg',
+);
+
+ok($app->get_config(),'config is retrieved when no min_app_githooks_version is specified');

This comment has been minimized.

@guillaumeaubert

guillaumeaubert Mar 16, 2017

Owner

Since we're testing that an exception isn't raised incorrectly, would it make sense to specifically test for that? e.g.:

lives_ok(
    sub {
        my $app = App::GitHooks->new(
            arguments => [],
            name      => 'commit-msg',
        );
    },
    'Config is retrieved when no min_app_githooks_version is specified',
);
@guillaumeaubert

guillaumeaubert Mar 16, 2017

Owner

Since we're testing that an exception isn't raised incorrectly, would it make sense to specifically test for that? e.g.:

lives_ok(
    sub {
        my $app = App::GitHooks->new(
            arguments => [],
            name      => 'commit-msg',
        );
    },
    'Config is retrieved when no min_app_githooks_version is specified',
);
+ name => 'commit-msg',
+);
+
+ok($app->get_config(),'config is retrieved when no min_app_githooks_version is specified');

This comment has been minimized.

@guillaumeaubert

guillaumeaubert Mar 16, 2017

Owner

new() calls get_config() so it's technically redundant when checking for the exception. On the other hand, explicitly calling the method where the check is makes the test less brittle. I would just be consistent across all three tests (either call get_config() everywhere or remove it entirely).

@guillaumeaubert

guillaumeaubert Mar 16, 2017

Owner

new() calls get_config() so it's technically redundant when checking for the exception. On the other hand, explicitly calling the method where the check is makes the test less brittle. I would just be consistent across all three tests (either call get_config() everywhere or remove it entirely).

lib/App/GitHooks.pm
@@ -837,6 +843,11 @@ sub get_config
);
}
+ # Enforce the specifying of min version of App::GitHooks

This comment has been minimized.

@guillaumeaubert

guillaumeaubert Mar 16, 2017

Owner

Please switch to hard tabs in the entire diff, for consistency with the rest of the distribution.

@guillaumeaubert

guillaumeaubert Mar 16, 2017

Owner

Please switch to hard tabs in the entire diff, for consistency with the rest of the distribution.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 17, 2017

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 2399f95 on lharey:master into 1399c6d on guillaumeaubert:master.

coveralls commented Mar 17, 2017

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 2399f95 on lharey:master into 1399c6d on guillaumeaubert:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 17, 2017

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 2399f95 on lharey:master into 1399c6d on guillaumeaubert:master.

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 2399f95 on lharey:master into 1399c6d on guillaumeaubert:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 17, 2017

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 2399f95 on lharey:master into 1399c6d on guillaumeaubert:master.

Coverage Status

Coverage increased (+0.03%) to 90.128% when pulling 2399f95 on lharey:master into 1399c6d on guillaumeaubert:master.

@lharey

This comment has been minimized.

Show comment
Hide comment
@lharey

lharey Mar 17, 2017

Contributor

Hi

I have updated as requested

Contributor

lharey commented Mar 17, 2017

Hi

I have updated as requested

@guillaumeaubert guillaumeaubert merged commit 5dbfd62 into guillaumeaubert:master Mar 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 90.128%
Details
@guillaumeaubert

This comment has been minimized.

Show comment
Hide comment
@guillaumeaubert

guillaumeaubert Mar 19, 2017

Owner

Thank you for adding this feature and for the corresponding tests, @lharey! Merged.

Owner

guillaumeaubert commented Mar 19, 2017

Thank you for adding this feature and for the corresponding tests, @lharey! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment