Skip to content

Commit

Permalink
Remove magic default value as per review comment
Browse files Browse the repository at this point in the history
  • Loading branch information
guillaume-d committed Oct 11, 2021
1 parent 9f1c8ba commit 3dc1d14
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 25 deletions.
17 changes: 8 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -685,33 +685,32 @@ with Overcommit without writing any Ruby code in a similar way as

These special line-aware command hooks behave and are configured the same way
as the Git ones, except only file arguments get passed to them.
Also to enable the feature, they must use at least one of the following options,
so that, using the command output:
Also to enable them and for optimal use, one must configure them as explained
below, so that, using the command output:
- differentiating between warnings and errors becomes possible
- modified lines can be detected and acted upon as defined by
the `problem_on_unmodified_line`, `requires_files`, `include` and `exclude`
[hook options](#hook-options)

**Warning**: Only the command's standard output stream is considered for now,
*not* its standard error stream.
If you do not need to change the default values for any other option,
then the `extract_messages_from` option has to be specified.
Its value does not matter for now, but it should be set to `stdout`
to avoid problems in the future.

To differentiate between warning and error messages,
the `warning_message_type_pattern` option may be specified:
the `type` field of the `message_pattern` regexp below must then include
the `warning_message_type_pattern` option's text.

The `message_pattern` option specifies the format of the command's messages.
It is a optional [(Ruby) regexp][RubyRE], which if present must at least define
It is mandatory, must be a [(Ruby) regexp][RubyRE], and must define at least
a `file` [named capture group][RubyRENCG].
The only other allowed ones are `line` and `type`, which when specified
enable detection of modified lines and warnings respectively.

**Note**: The default value for this option is often adequate:
it generalizes the quasi-standard [GNU/Emacs-style error format][GNUEerrf],
**Tip**: The following value for this option is often adequate:
```ruby
/^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]* (?<type>[^ ]+)/
```
It generalizes the quasi-standard [GNU/Emacs-style error format][GNUEerrf],
adding the most frequently used warning syntax to it.

For example:
Expand Down
11 changes: 2 additions & 9 deletions lib/overcommit/hook_loader/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,8 @@ def load_hooks

private

# GNU/Emacs-style error format:
AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN =
/^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]* (?<type>[^ ]+)/.freeze

def is_hook_line_aware(hook_config)
hook_config['extract_messages_from'] ||
hook_config['message_pattern'] ||
hook_config['warning_message_type_pattern']
hook_config['message_pattern']
end

def create_line_aware_command_hook_class(hook_base) # rubocop:disable Metrics/MethodLength
Expand All @@ -53,8 +47,7 @@ def extract_messages(line_aware_config, result)
warning_message_type_pattern = line_aware_config[:warning_message_type_pattern]
Overcommit::Utils::MessagesUtils.extract_messages(
result.stdout.split("\n"),
line_aware_config[:message_pattern] ||
AD_HOC_HOOK_DEFAULT_MESSAGE_PATTERN,
line_aware_config[:message_pattern],
Overcommit::Utils::MessagesUtils.create_type_categorizer(
warning_message_type_pattern
)
Expand Down
15 changes: 8 additions & 7 deletions spec/overcommit/hook_loader/plugin_hook_loader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,18 @@
flags:
- "--format=emacs"
include: '**/*.foo'
FooLintDefault:
FooLintRecommendedPattern:
enabled: true
command: ["foo", "lint"]
message_pattern: !ruby/regexp /^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]* (?<type>[^ ]+)/
warning_message_type_pattern: warning
flags:
- "--format=emacs"
include: '**/*.foo'
FooLintDefaultNoWarnings:
FooLintRecommendedPatternNoWarnings:
enabled: true
command: ["foo", "lint"]
extract_messages_from: stdout
message_pattern: !ruby/regexp /^(?<file>(?:\w:)?[^:]+):(?<line>\d+):[^ ]* (?<type>[^ ]+)/
flags:
- "--format=emacs"
include: '**/*.foo'
Expand Down Expand Up @@ -143,8 +144,8 @@
it { should fail_hook }
end

describe '(using default pattern)' do
let(:hook_name) { 'FooLintDefault' }
describe '(using recommended pattern)' do
let(:hook_name) { 'FooLintRecommendedPattern' }

context 'when command fails with some warning message' do
let(:result) do
Expand Down Expand Up @@ -175,8 +176,8 @@
end
end

describe '(using defaults)' do
let(:hook_name) { 'FooLintDefaultNoWarnings' }
describe '(using recommended pattern w/o warnings)' do
let(:hook_name) { 'FooLintRecommendedPatternNoWarnings' }

context 'when command fails with some messages' do
let(:result) do
Expand Down

0 comments on commit 3dc1d14

Please sign in to comment.