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

Allow a file with a list of files as input #201

Merged
merged 3 commits into from
May 6, 2016
Merged

Conversation

lpantano
Copy link
Contributor

@lpantano lpantano commented May 5, 2016

Hi!

this is a suggestion PR, since probably you have a better idea how to integrate it.

But we had some problems with a person with so many arguments (files) in the command line that it cannot run. (bcbio/bcbio-nextgen#1380)

In bcbio, we are moving to CWL, what has many advantages, but one of the inconvenient is that we cannot use commands assuming that we are in the same machine or in a standard file system. So we can not use multiqc qc/* etc. Instead we are passing the specific path.

So, @chapmanb thought that maybe it would be good to have an argument to allow a file that contains a list of files to be used by multiqc.

As I said, this is a suggestion, because I like to offer some kind of solution instead of just asking, but feel free to adapt however you think is better.

thanks!

PS: another problem it is if 700 samples make any sense inside multiqc, but I think we can change things to allow a proper report in the future.

@ewels
Copy link
Member

ewels commented May 5, 2016

Aha, yes I can see how that could be a problem. I can't say that I ever expected anyone to specify every single log file :)

This should be fairly easy to implement and your changes look good. I might suggest a couple of minor changes in strategy in inline comments though if that's ok 😉

Note that I'm probably going to rewrite this code fairly soon. You just prompted me to write down my intentions, so now described in #202. Shouldn't affect this change though.

Phil

@ewels
Copy link
Member

ewels commented May 5, 2016

Overarching code suggestions in one place here first:

  • Rename list_files to file_list for clarity
  • Make it a boolean flag instead of string
  • Keep the analysis_dir field required and use this to get the path to the file list
    • Check that only one path specified if file_list is true.

Will follow these up with inline comments.

@@ -27,6 +27,7 @@
creation_date = datetime.now().strftime("%Y-%m-%d, %H:%m")
working_dir = os.getcwd()
analysis_dir = [os.getcwd()]
list_files = None
Copy link
Member

Choose a reason for hiding this comment

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

file_list = False

@ewels ewels added this to the v0.7 milestone May 5, 2016
@ewels
Copy link
Member

ewels commented May 5, 2016

Oh, and please also add the parameter to the example config file and add a bit to the docs about it if that's ok. Thanks!

Phil

@ewels
Copy link
Member

ewels commented May 5, 2016

Actually, perhaps my suggestion about using analysis_dir is dump, not sure. You could keep file_list as specifying the path to the file, maybe more intuitive. Click has an option for file where it checks that it exists though, so some functionality for free there.

@lpantano
Copy link
Contributor Author

lpantano commented May 6, 2016

I followed your first comment to convert file_list in a flag. I liked more, but is not perfect either of them. Let me know what you think now. Maybe we can leave it as a flag, and change it later depending of the user suggestions ...

Thanks!!!!

if is_file_list:
if len(analysis_dir) > 1:
raise ValueError("If --is-file-list is giving, analysis_dir should have only one plain text file.")
if os.path.isdir(analysis_dir[0]):
Copy link
Member

Choose a reason for hiding this comment

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

You can probably skip this bit - the open command will throw an exception if it's a directory anyway I think.

@ewels
Copy link
Member

ewels commented May 6, 2016

Looks good! Couple of minor comments but then will merge. We also need to add a demo file list to MultiQC_TestData and update .travis.yaml to add a test for this new feature.

@ewels
Copy link
Member

ewels commented May 6, 2016

Ok, added a test file so you should now be able to add a test command to the travis config:

multiqc --file_list data/special_cases/file_list.txt

- multiqc --is-file-list data/special_cases/file_list.txt
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, you'll need -f here to get it to overwrite the existing report.

@ewels
Copy link
Member

ewels commented May 6, 2016

Sorry, just saw this. Looks good!

@ewels ewels merged commit bae7701 into MultiQC:master May 6, 2016
@ewels
Copy link
Member

ewels commented May 6, 2016

Thanks!

@ewels ewels mentioned this pull request May 18, 2016
@ewels
Copy link
Member

ewels commented May 23, 2016

Hi @lpantano,

I just changed --is-file-list to the simpler --file-list, hope this is ok!

Phil

@ewels
Copy link
Member

ewels commented May 23, 2016

Sorry, shouldn't push changes when I'm tired! New commit with changes actually working now.

@lpantano
Copy link
Contributor Author

thanks!

vladsavelyev pushed a commit that referenced this pull request Feb 16, 2024
…-3.1-and-lt-5.0

Update flake8-debugger requirement from ~=3.1 to >=3.1,<5.0
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.

None yet

2 participants