Extract Out PreCommit Message Methods to Utils

[taufek]

Extracted messages methods in PreCommit context to Utils module.

This is a step closer to make these PreCommit hooks usable on other context.

[sds]

Thanks for this PR, @taufek.

There's opportunity to reduce the scope of this change and clean up some unnecessary abstraction, but I like the direction this takes us with respect to opening up these helpers to other hooks.

[sds]

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.

[taufek]

Sounds good to me.

[sds]

It's not clear to me why we're creating an inner class via a Struct that shares the same name as the MessageProcessor class, even if it is isolated. Rather than create a struct and call process on it, why not just get rid of the struct and run:

# Replace `MessageProcessor.new(message, regex, type_categorizer, index).process` with:
process_message(message, regex, type_categorizer, index)

...and redefine the process method below to take arguments.

This allows you to have the remaining methods in this file as regular methods. They can remain private and isolated from the outside world, since you're using class << self above.

[taufek]

I do agree it is a poor choice for naming the Struct.

The reason why I created an object to process each message within the loop is to avoid passing the context to the methods.

extract_type(match, message)

extract_file(match, message)

extract_line(match, message)

If we move the methods to an object at individual message level we will methods without argument because now all the previous arguments are accessible via instance variables.

extract_type

extract_file

extract_line

This way we distinguish the methods between the list level (messages) and indivual item level ( a message)

But if you still prefer to keep the methods with arguments I will change it.

[sds]

I can see the reason why using a Struct can save you from having to pass arguments explicitly, but I don't think this really gets you much, as it hides the dependencies of the method (i.e. what it actually needs to operate).

Method calls with arguments passed to them give you much more information about what the method is actually doing, rather than having it read a number of instance variables internal to the object.

[taufek]

Understood.

[taufek]

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

[sds]

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.

[taufek]

Noted.

[taufek]

@sds ,

This is ready for re-review. Thanks for the feedback.

[sds]

Thanks for addressing those comments, @taufek!