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

Set several modules too-global search patterns to share #727

Merged

Conversation

cpavanrun
Copy link
Contributor

Went over the search_patterns.yaml .

Those modules which entries I thought too global had shared: true added to them in order to prevent them from incorectly claiming a file. Mostly these are entries with fn without any content.

This should reduce the number of times a module eats a log when it's search pattern had a false-positive hit. I don't think this will result in too much of a performance hit.

Cheers

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

I agree that many of these are too general. Instead of using shared: true we could also require specific file contents. This may be more specific and also faster.. What do you think?

afterqc:
fn: '*.json'
contents: 'allow_mismatch_in_poly'
shared: true
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed, as it also needs the string allow_mismatch_in_poly in the file which should be specific enough (it's not claiming all JSON files).

@ewels ewels added the waiting: changes Issue / PR is on hold, waiting for requested changes label Apr 24, 2018
@cpavanrun
Copy link
Contributor Author

cpavanrun commented May 22, 2018

Sorry, had this laying around for a bit tooooo long =/.

I agree that making the search-pattern entries more specific via file contents would be the better solution. However, this is not possible for ALL the too-global modules. A balance has been struck in 3086e73.

A wise lesson to all tool developers: add some freaking commented header metadata to your output files.

@ewels ewels removed the waiting: changes Issue / PR is on hold, waiting for requested changes label Jul 3, 2018
@ewels
Copy link
Member

ewels commented Jul 12, 2018

Great stuff, thank you for this! Relatively minor changes, but could actually solve a lot of "bugs" for a lot of people! 👍

@ewels ewels merged commit f1dab4b into MultiQC:master Jul 12, 2018
ewels added a commit that referenced this pull request Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants