Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add matcher config in configuration file #1176

Merged
merged 3 commits into from
Jan 1, 2021
Merged

Add matcher config in configuration file #1176

merged 3 commits into from
Jan 1, 2021

Conversation

mbj
Copy link
Owner

@mbj mbj commented Dec 23, 2020

No description provided.

@mbj mbj requested a review from dgollahon January 1, 2021 03:55
@mbj mbj marked this pull request as ready for review January 1, 2021 03:55
@mbj mbj force-pushed the add/matcher-config branch 3 times, most recently from af24813 to c899bf9 Compare January 1, 2021 03:59
docs/configuration.md Outdated Show resolved Hide resolved
@mbj mbj force-pushed the add/matcher-config branch 4 times, most recently from 79dc889 to 53b9e40 Compare January 1, 2021 04:03
@@ -159,13 +112,14 @@ def self.env
Transform::Exception.new(YAML::SyntaxError, YAML.method(:safe_load)),
Transform::Hash.new(
optional: [
Transform::Hash::Key.new('coverage_criteria', CoverageCriteria::TRANSFORM),
Transform::Hash::Key.new('coverage_criteria', ->(value) { CoverageCriteria::TRANSFORM.call(value) }),
Copy link
Owner Author

Choose a reason for hiding this comment

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

I "really" like this require sequence:

require 'foo'
require 'foo/bar'

But we have to access the constant from foo/bar here, hence as we are in class scope and the require for `foo/bar' did not happen yet we have to use a lambda indirection.

The alternative would be:

require 'foo/bar'
require 'foo'

which sucks. For the moment I eat it. Would love of Ruby had a module system ;).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would love of Ruby had a module system ;)

yeahh... I feel that one!

@@ -64,7 +64,7 @@ This prints a report like:

```sh
Mutant environment:
Matcher: #<Mutant::Matcher::Config match_expressions: [AUOM*]>
Matcher: #<Mutant::Matcher::Config subjects: [AUOM*]>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good change I think--it reads more clearly.

@@ -64,7 +64,7 @@ This prints a report like:

```sh
Mutant environment:
Matcher: #<Mutant::Matcher::Config match_expressions: [AUOM*]>
Matcher: #<Mutant::Matcher::Config subjects: [AUOM*]>
Copy link
Owner Author

Choose a reason for hiding this comment

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

I think this is much nicer to read. Its clear the domain of this method is matching, else the class would not be named Mutant::Matcher.

So the match_ prefix was redundant always.

Just naming the key expressions would IMO also suck as it than would have a sibling ignore_expressions which does not clearly indicate what the difference between both would be.

Hence replacing this with subjects makes it very clear. And subject_expressions is kind of redundant as we can only match expressions.

@@ -41,6 +41,7 @@ module Mutant
SCOPE_OPERATOR = '::'
end # Mutant

require 'mutant/transform'
Copy link
Owner Author

Choose a reason for hiding this comment

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

The fact this has to move up the require chain to toplevel, indicates it likely belongs to a separate library.

Once I make nicer "input error location rendering" this is worthy of a separate lib.

Transform::Hash::Key.new('fail_fast', Transform::BOOLEAN),
Transform::Hash::Key.new('includes', Transform::STRING_ARRAY),
Transform::Hash::Key.new('integration', Transform::STRING),
Transform::Hash::Key.new('jobs', Transform::INTEGER),
Transform::Hash::Key.new('mutation_timeout', Transform::FLOAT),
Transform::Hash::Key.new('requires', Transform::STRING_ARRAY)
Transform::Hash::Key.new('requires', Transform::STRING_ARRAY),
Transform::Hash::Key.new('matcher', Matcher::Config::LOADER)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This one, paired with adding the constant Mutant::Config::LOADER is the only real change to make config file support for matchers.

dgollahon
dgollahon previously approved these changes Jan 1, 2021
Copy link
Collaborator

@dgollahon dgollahon left a comment

Choose a reason for hiding this comment

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

Woo! I'm excited about this one 🎉 LGTM assuming CI passes.

@mbj
Copy link
Owner Author

mbj commented Jan 1, 2021

Woo! I'm excited about this one LGTM assuming CI passes.

it should, I repushed a lot so have to manage the queue. Merging once done.

* This class needs to traverse the process tree often, hence it tries to
  minimize its `marshal` representation.
* For that reason we do not even use `adamantium` to avoid the memory.
* A custom self freeze `.new` method was added instead.
* That clashes with the one `abstract_type` installs before the
  re-definition of `.new`. Turning `abstract_type` semantics unobservable.
* Removing abstract type for this class is the local optimum for the
  moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants