Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 3 additions & 65 deletions lib/overcommit/hook/pre_commit/base.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'forwardable'
require 'overcommit/utils/messages_utils'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sds ,

I have a question on this file loading. It is possible to avoid defining this and use autoloading instead? I notice modules like GitRepo and FileUtils we can use them via autoloading.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are not using autoloading anywhere else in this project, I would prefer not to introduce it at this time. I've been bitten by odd loading errors when trying to be too clever (usually not problems caused by autoloading itself, but by how others load a library locally). If someone uses your code in a way you didn't expect, they could trigger "autoloading" of classes at a time that you don't expect, which can cause difficult to debug issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted.


module Overcommit::Hook::PreCommit
# Functionality common to all pre-commit hooks.
Expand All @@ -9,71 +10,8 @@ class Base < Overcommit::Hook::Base

private

# Extract file, line number, and type of message from an error/warning
# messages in output.
#
# Assumes each element of `output` is a separate error/warning with all
# information necessary to identify it.
#
# @param output_messages [Array<String>] unprocessed error/warning messages
# @param regex [Regexp] regular expression defining `file`, `line` and
# `type` capture groups used to extract file locations and error/warning
# type from each line of output
# @param type_categorizer [Proc] function executed against the `type`
# capture group to convert it to a `:warning` or `:error` symbol. Assumes
# `:error` if `nil`.
# @raise [Overcommit::Exceptions::MessageProcessingError] line of output did
# not match regex
# @return [Array<Message>]
def extract_messages(output_messages, regex, type_categorizer = nil)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than deleting this entirely (and thus requiring all the hooks to reference the new Overcommit::Utils::MessagesUtils module), you could leave the method in place and call the helper.

def extract_messages(*args)
  Overcommit::Utils::MessagesUtils.extract_messages(*args)
end

This also avoids breaking existing custom hooks in the wild that depend on extract_messages. Let's minimize disruption and keep the change minimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

output_messages.map.with_index do |message, index|
unless match = message.match(regex)
raise Overcommit::Exceptions::MessageProcessingError,
'Unexpected output: unable to determine line number or type ' \
"of error/warning for output:\n" \
"#{output_messages[index..-1].join("\n")}"
end

file = extract_file(match, message)
line = extract_line(match, message) if match.names.include?('line') && match[:line]
type = extract_type(match, message, type_categorizer)

Overcommit::Hook::Message.new(type, file, line, message)
end
end

def extract_file(match, message)
return unless match.names.include?('file')

if match[:file].to_s.empty?
raise Overcommit::Exceptions::MessageProcessingError,
"Unexpected output: no file found in '#{message}'"
end

match[:file]
end

def extract_line(match, message)
return unless match.names.include?('line')
Integer(match[:line])
rescue ArgumentError, TypeError
raise Overcommit::Exceptions::MessageProcessingError,
"Unexpected output: invalid line number found in '#{message}'"
end

def extract_type(match, message, type_categorizer)
if type_categorizer
type_match = match.names.include?('type') ? match[:type] : nil
type = type_categorizer.call(type_match)
unless Overcommit::Hook::MESSAGE_TYPES.include?(type)
raise Overcommit::Exceptions::MessageProcessingError,
"Invalid message type '#{type}' for '#{message}': must " \
"be one of #{Overcommit::Hook::MESSAGE_TYPES.inspect}"
end
type
else
:error # Assume error since no categorizer was defined
end
def extract_messages(*args)
Overcommit::Utils::MessagesUtils.extract_messages(*args)
end
end
end
75 changes: 75 additions & 0 deletions lib/overcommit/utils/messages_utils.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
module Overcommit::Utils
# Utility to process messages
module MessagesUtils
class << self
# Extract file, line number, and type of message from an error/warning
# messages in output.
#
# Assumes each element of `output` is a separate error/warning with all
# information necessary to identify it.
#
# @param output_messages [Array<String>] unprocessed error/warning messages
# @param regex [Regexp] regular expression defining `file`, `line` and
# `type` capture groups used to extract file locations and error/warning
# type from each line of output
# @param type_categorizer [Proc] function executed against the `type`
# capture group to convert it to a `:warning` or `:error` symbol. Assumes
# `:error` if `nil`.
# @raise [Overcommit::Exceptions::MessageProcessingError] line of output did
# not match regex
# @return [Array<Message>]
def extract_messages(output_messages, regex, type_categorizer = nil)
output_messages.map.with_index do |message, index|
unless match = message.match(regex)
raise Overcommit::Exceptions::MessageProcessingError,
'Unexpected output: unable to determine line number or type ' \
"of error/warning for output:\n" \
"#{output_messages[index..-1].join("\n")}"
end

file = extract_file(match, message)
line = extract_line(match, message) if match.names.include?('line') && match[:line]
type = extract_type(match, message, type_categorizer)

Overcommit::Hook::Message.new(type, file, line, message)
end
end

private

def extract_file(match, message)
return unless match.names.include?('file')

if match[:file].to_s.empty?
raise Overcommit::Exceptions::MessageProcessingError,
"Unexpected output: no file found in '#{message}'"
end

match[:file]
end

def extract_line(match, message)
return unless match.names.include?('line')
Integer(match[:line])
rescue ArgumentError, TypeError
raise Overcommit::Exceptions::MessageProcessingError,
"Unexpected output: invalid line number found in '#{message}'"
end

def extract_type(match, message, type_categorizer)
if type_categorizer
type_match = match.names.include?('type') ? match[:type] : nil
type = type_categorizer.call(type_match)
unless Overcommit::Hook::MESSAGE_TYPES.include?(type)
raise Overcommit::Exceptions::MessageProcessingError,
"Invalid message type '#{type}' for '#{message}': must " \
"be one of #{Overcommit::Hook::MESSAGE_TYPES.inspect}"
end
type
else
:error # Assume error since no categorizer was defined
end
end
end
end
end