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

multiqc: config: Catch FileNotFoundError when git is not installed. #377

Closed
wants to merge 1 commit into from

Conversation

wwood
Copy link

@wwood wwood commented Dec 26, 2016

Without this patch, the incorrect exception is caught when git is not in PATH.

@ewels
Copy link
Member

ewels commented Dec 26, 2016

Ah rubbish, I hadn't thought of this scenario. Many thanks @wwood. Note that the FileNotFoundError exception type doesn't seem to be available in Python 2 though.

Phil

@wwood
Copy link
Author

wwood commented Dec 30, 2016

So travis says. Well, for the purposes of Guix, that doesn't matter since python3 is used for running multiqc. I'm also not sure of the best way to handle this 2/3 problem, so I'll leave it to others to fix more generally.

Thanks for the quick response.

@ewels
Copy link
Member

ewels commented Jan 1, 2017

Thanks @wwood - I'm away on holiday now and won't be back until mid-February. A quick google suggests that using the base exception class IOError instead of FileNotFound should work for both Python 2 and 3, but I can't test here unfortunately. If you update your PR with that and Travis passes I'd be happy to merge from here though.

Cheers,

Phil

ewels added a commit that referenced this pull request Feb 20, 2017
@ewels ewels closed this Feb 20, 2017
@ewels
Copy link
Member

ewels commented Feb 20, 2017

Hi @wwood,

I've just pushed a change which just handles any exception triggered by this code. Not great coding practice, but as this is such an unimportant feature I don't think that it should ever halt the execution of MultiQC. As a bonus, it should work with both Python 2 and 3 😅

Hope this is ok, let me know how you get on with the change. Thanks again for the PR..

Phil

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.

None yet

2 participants