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

feat(exclude-files): Exclude files with a ! #188

Merged
merged 5 commits into from
Dec 15, 2016
Merged

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Dec 14, 2016

Add a way to exclude files. This works respecting the order of the file patterns. Only works when providing patterns as a string, not for the InputFileDescriptor format.

  • Refactor InputFileResolver, included a new class PatternResolver in order to reuse functionality for both mutate and files.
  • Ignore patterns starting with an ! as long as they are provided as strings
  • Deduplicate files, first occurance wins.
  • Document behavior in the readme and add example to cli help text

As discussed in stryker-api#10

* Refactor `InputFileResolver`, included a new class `PatternResolver` in order to reuse functionality for both `mutate` and `files`.
* Ignore patterns starting with an `!` as long as they are provided as strings
* Deduplicate files, first occurance wins.
@simondel simondel self-assigned this Dec 14, 2016
@simondel
Copy link
Member

This should fix #172 and #95

Copy link
Member

@simondel simondel left a comment

Choose a reason for hiding this comment

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

Could you take a look at my two comments?

});
}

function markAdditionalFilesToMutate(allInputFiles: InputFile[], additionalMutateFiles: string[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are many of these functions outside of a class definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason. I'll move them back up. Of course they don't need to be part of the class, but keeping them as static methods does keep them nicely in the class definition yes.

additionalMutateFiles.forEach(mutateFile => {
if (!allInputFiles.filter(inputFile => inputFile.path === mutateFile).length) {
errors.push(`Could not find mutate file "${mutateFile}" in list of files.`);
constructor(descriptor: InputFileDescriptor | string, private previous?: PatternResolver) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you choose an implementation that gave every PatternResolver a link to the previous one? For me, it would make more sense when the InputFileResolver would combine each of the PatternResolvers, while the PatternResolver now does it himself.

Copy link
Member Author

Choose a reason for hiding this comment

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

This way we can keep the InputFileResolver clean by moving part of the logic to PatternResolver. It's a more functional approach, right in line with my current experience programming Scala ;).

I can see where you're coming from, do you want me to move it back to the InputFileResolver?

return inputFiles;
});
}

private static validateFileDescriptor(maybeInputFileDescriptors: Array<InputFileDescriptor | string>) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still have to keep all of these methods static? They only seem to be called from inside non-static functions and they're private.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but they don't use the this pointer so they can be static. Rather see them as instance functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved them to be instance methods. Looks a bit cleaner, i agree.

* Move static methods to be instance methods
@simondel
Copy link
Member

I'll be so glad when it's 2017 and we can drop Node 0.12 support because it keeps failing our build

@simondel simondel merged commit 05a356d into master Dec 15, 2016
@simondel simondel deleted the feat-exclude-files branch December 15, 2016 12:30
@nicojs
Copy link
Member Author

nicojs commented Dec 15, 2016

Released in 0.5.4

@nicojs nicojs mentioned this pull request Dec 15, 2016
@nicojs
Copy link
Member Author

nicojs commented Dec 21, 2016

We should remove the support next week. You know... as a new years gift to ourselves 😁

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