Altered if clasuses around selection of commands in umi_tools.py #537

IanSudbery · 2022-06-05T17:34:21Z

Fixes (or at least starts to fix) #536

@TomSmithCGAT Bit confused as to what is going on in the umi_tools module. Specifically what the original purpose of the if clause at lines 51-52 was.

I've also made each tool print its usage message if no options are passed at all. Don't know if this was the right was to do this, or if I should have done something more central (although that would become a problem if there was a tool that could be run without options. Which currently there isn't).

TomSmithCGAT · 2022-06-08T08:00:31Z

umi_tools/Utilities.py

@@ -1030,7 +1030,7 @@ def Start(parser=None,

    if return_parser:
        return parser
-
+       


Is that just a tab added?

TomSmithCGAT · 2022-06-08T08:00:59Z

umi_tools/count_tab.py

@@ -86,12 +86,17 @@ def main(argv=None):

    if argv is None:
        argv = sys.argv
-
+        


Another tab to remove?

There are a few of these in the PR. One in each tool it looks like. Unless I'm missunderstanding the github diff markup.

I don't think you are misunderstanding.

TomSmithCGAT · 2022-06-08T08:01:26Z

umi_tools/count_tab.py

    # setup command line parser
    parser = U.OptionParser(version="%prog version: $Id$",
                            usage=usage,
                            description=globals()["__doc__"])

+    if len(argv) == 1:


Happy with this to catch use of tool with no options

TomSmithCGAT · 2022-06-08T08:19:33Z

umi_tools/umi_tools.py

@@ -50,12 +48,22 @@ def main(argv=None):

        return

-    elif argv[2] in ["--help", "-h", "--help-extended"]:
+    elif len(argv) > 2 and  argv[2] in ["--help", "-h", "--help-extended"]:


I think this elif can be removed. The tools all have -h/--help/--help-extended options so no need to catch anything here

My guess is that this was added because the standard --help within each tool doesn't output the version number?

Ah, maybe.

On a related note, the --version options for each tool also don't properly return the version. Should probably just remove that option from the tools and make it available via umi_tools --version only

TomSmithCGAT · 2022-06-08T08:29:49Z

Hey @IanSudbery. Assuming you're referring to the below and the line numbers are from this PR?

UMI-tools/umi_tools/umi_tools.py

Lines 53 to 54 in 0c72fa9

    
           elif argv[2] in ["--help", "-h", "--help-extended"]: 
        
               print("UMI-Tools: Version %s" % __version__)

I'm not sure what was going on with that elif either. It looks like I added it when we split into multiple tools to print help when a tool is specified and then you changed the output to be the version number (0101494 ). Regardless, each tool command line has help options added so no need to catch it in umi_tools.py.

I'm fine with having the same code in each tool to output help if no options are given. I agree it would be best done centrally though if you can be bothered recoding. I guess the easiest way to do it would be to catch instances where len(sys.args)==2 after the checks for version/help options, print the line you have about needing to add required options and then add --help to the sys.args and call the tool module main. I can't imagine us adding a tool with no help in the future since our base arg parser adds --help anyway

TomSmithCGAT · 2022-06-08T08:32:29Z

Can't see how to comment on lines that haven't been updated in the PR but L46 in umi_tools.py:

elif len(argv) == 1 or argv[1] == "--version" or argv[1] == "-v":

That len(argv) == 1 can go, since that case is met by the if and causes the help to be printed.

Minor thing.

TomSmithCGAT · 2022-06-08T08:36:00Z

umi_tools/umi_tools.py

+        module = importlib.import_module("umi_tools." + command, "umi_tools")
+    except ImportError:
+        print("Command %s not recognosied" % command)
+        print()


What's the print() for?

IanSudbery · 2022-06-08T08:50:33Z

can't imagine us adding a tool with no help in the future since our base arg parser adds --help anyway

Thats not what I meant. I guess I was thinking about a tool that had no required arguements. For example, you could imagine that if extract had a default --pattern it could be run:

cat my_reads.fastq | umi_tools extract > my_reads.extracted.fastq

Although of course it doesn't, and I think all our tools require options of some sort (although come to think of it the new prepare_for_rsem or whatever we are going to call it might not).

In the above len(argv) == 1, but there is nothing wrong with it. Although perhaps this is just premature optimisation on my part.

I'll clean up the rough tabs.

TomSmithCGAT · 2022-06-08T09:03:24Z

Ah, got it. Yeah, that is a good point. Probably being optimistic about future tools being added 😉

IanSudbery · 2022-06-08T11:24:20Z

Whitespace should be tidied up.

TomSmithCGAT · 2022-06-08T13:31:33Z

Noticed in passing that a lot of the changes to umi_tools.py are also in #506. Should we first merge that one so that the contributions include @[epruesse]?

IanSudbery · 2022-06-08T14:41:31Z

yes Ian Sudbery Senior Lecturer in Bioinformatics, Sheffield Institute for Nucleic Acids, School of Biosciences, The University of Sheffield. web: www.sudlab.co.uk Tel: 0114 222 2738 Twitter: IanSudbery Show Calendar Availability <https://calendar.google.com/calendar/u/0?cid=aS5zdWRiZXJ5QHNoZWZmaWVsZC5hYy51aw>

…

On Wed, 8 Jun 2022 at 14:31, Tom Smith ***@***.***> wrote: Noticed in passing that a lot of the changes to umi_tools.py are also in #506 <#506>. Should we first merge that one so that the contributions include @[epruesse]? — Reply to this email directly, view it on GitHub <#537 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJELDS3HG4H2VSPTZ4G5FDVOCOEBANCNFSM5X5LFZZA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

TomSmithCGAT · 2022-09-26T09:49:54Z

@IanSudbery - This is OK to merge, right?

IanSudbery added 2 commits June 5, 2022 17:59

Altered if clasuses around selection of commands in umi_tools.py

0470bdb

Added default usage message on not options

0762a0a

IanSudbery requested a review from TomSmithCGAT June 5, 2022 17:34

TomSmithCGAT reviewed Jun 8, 2022

View reviewed changes

TomSmithCGAT closed this Jun 8, 2022

TomSmithCGAT reopened this Jun 8, 2022

TomSmithCGAT reviewed Jun 8, 2022

View reviewed changes

IanSudbery added 3 commits June 8, 2022 12:16

Whitespace

16fa2fe

More Whitespace

82a44f0

More Whitespace

9302f51

TomSmithCGAT approved these changes Jun 8, 2022

View reviewed changes

Merge branch 'master' into {IS}_fix_help_messages

525c38b

IanSudbery merged commit a5b3489 into master Sep 26, 2022

TomSmithCGAT deleted the {IS}_fix_help_messages branch October 3, 2024 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Altered if clasuses around selection of commands in umi_tools.py #537

Altered if clasuses around selection of commands in umi_tools.py #537

IanSudbery commented Jun 5, 2022

TomSmithCGAT Jun 8, 2022

TomSmithCGAT Jun 8, 2022

TomSmithCGAT Jun 8, 2022

IanSudbery Jun 8, 2022

TomSmithCGAT Jun 8, 2022

TomSmithCGAT Jun 8, 2022

IanSudbery Jun 8, 2022

TomSmithCGAT Jun 8, 2022

TomSmithCGAT commented Jun 8, 2022 •

edited

Loading

TomSmithCGAT commented Jun 8, 2022

TomSmithCGAT Jun 8, 2022

IanSudbery commented Jun 8, 2022

TomSmithCGAT commented Jun 8, 2022

IanSudbery commented Jun 8, 2022

TomSmithCGAT commented Jun 8, 2022

IanSudbery commented Jun 8, 2022 via email

TomSmithCGAT commented Sep 26, 2022

		@@ -1030,7 +1030,7 @@ def Start(parser=None,

		if return_parser:
		return parser

		@@ -86,12 +86,17 @@ def main(argv=None):

		if argv is None:
		argv = sys.argv

Altered if clasuses around selection of commands in umi_tools.py #537

Altered if clasuses around selection of commands in umi_tools.py #537

Conversation

IanSudbery commented Jun 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSmithCGAT commented Jun 8, 2022 • edited Loading

TomSmithCGAT commented Jun 8, 2022

Choose a reason for hiding this comment

IanSudbery commented Jun 8, 2022

TomSmithCGAT commented Jun 8, 2022

IanSudbery commented Jun 8, 2022

TomSmithCGAT commented Jun 8, 2022

IanSudbery commented Jun 8, 2022 via email

TomSmithCGAT commented Sep 26, 2022

TomSmithCGAT commented Jun 8, 2022 •

edited

Loading