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

Don't sys.exit from run() function #2055

Merged
merged 4 commits into from Sep 21, 2023
Merged

Don't sys.exit from run() function #2055

merged 4 commits into from Sep 21, 2023

Conversation

ewels
Copy link
Member

@ewels ewels commented Sep 19, 2023

Noticed that we have a stray sys.exit() call in the run() function. This should be removed for usage in interactive environments.

Not checked thoroughly yet, so needs sanity checking / testing before merge.

@ewels ewels added this to the MultiQC v1.16 milestone Sep 19, 2023
@ewels
Copy link
Member Author

ewels commented Sep 20, 2023

Need to double check that I haven't missed any other sys.exit() calls here.

@ewels
Copy link
Member Author

ewels commented Sep 20, 2023

Scooped up a couple of others, some remain but I think that we can tackle those during wider refactoring. Hopefully this PR replaces the ones within multiqc.run() which is the priority here.

Still needs testing, running each of the edge cases where these trigger, to make sure I haven't screwed up.

  • No modules specified
  • No analysis results found
  • Report filename already exists

@@ -981,7 +981,7 @@ def __rich_measure__(self, console: rich.console.Console, options: rich.console.
logger.error("Output directory {} already exists.".format(config.plots_dir))
logger.info("Use -f or --force to overwrite existing reports")
shutil.rmtree(tmp_dir)
sys.exit(1)
return {"report": report, "config": config, "sys_exit_code": 1}
Copy link
Member

@vladsavelyev vladsavelyev Sep 20, 2023

Choose a reason for hiding this comment

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

I guess because of these lines it also should never happen, unless a folder was created by another process.
https://github.com/ewels/MultiQC/blob/5f21841908b3771d9775a97521faf0a9737ad5d6/multiqc/multiqc.py#L933-L934

Simulated that, works fine

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, you might be right there. I suspect that this code predates the rotating suffix thing being added. In which case, we can presumably just delete this chunk of code.

Copy link
Member

@vladsavelyev vladsavelyev left a comment

Choose a reason for hiding this comment

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

Approving, works as expected. Will do more notebook environment testing when the refactoring is complete

@ewels
Copy link
Member Author

ewels commented Sep 20, 2023

Feel free to merge when you're happy @vladsavelyev 👍🏻

@vladsavelyev vladsavelyev merged commit bf7d471 into master Sep 21, 2023
10 checks passed
@vladsavelyev vladsavelyev deleted the sys-exit branch September 21, 2023 09:27
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