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

Add CLI #86

Merged
merged 12 commits into from
Oct 28, 2022
Merged

Add CLI #86

merged 12 commits into from
Oct 28, 2022

Conversation

prisae
Copy link
Collaborator

@prisae prisae commented Jul 22, 2022

Command-line interface CLI

Initial implementation - very basic, no tests so far. I am more interested to know first if you are interested in this at all (@banesullivan, @akaszynski).

What you can do currently in a terminal:

Print the default scooby-report

scooby

Print only the version of scooby

scooby --version

Print the help

scooby --help

Print any number of packages (they are added to the additional list)

scooby xarray h5py

Also implemented is the sort-flag. It is false by default (just as in the Python interface), and if you specify it it will be set to true.

scooby xarray h5py --sort

The implementation is quite flexible and works for:

  1. calling it in a terminal anywhere (the installed version of scooby)
  2. calling the scooby folder (that is the reason for the scooby/__main__.py-file)
  3. calling directly scooby.__main__.py

Although, frankly, only (1) is of general interest. (2) and (3) can be useful when developing or testing.

TODO's

  • Add documentation to the README
  • Add tests
  • conda-recipe needs updating

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2022

Codecov Report

Merging #86 (a359433) into main (65f2667) will decrease coverage by 0.98%.
The diff coverage is 75.00%.

❗ Current head a359433 differs from pull request most recent head 7940dfd. Consider uploading reports for the commit 7940dfd to get more accurate results

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
- Coverage   86.59%   85.61%   -0.99%     
==========================================
  Files           4        5       +1     
  Lines         388      424      +36     
==========================================
+ Hits          336      363      +27     
- Misses         52       61       +9     

@prisae
Copy link
Collaborator Author

prisae commented Jul 22, 2022

Using the CLI you can really see the improvement made by #85

Without faster-import
without-faster-import

WITH faster-import
with-faster-import

It obviously still has some cost, what we can see when comparing it to starting python with a plain print-statement:
plain

@banesullivan
Copy link
Owner

I'm generally in favor of adding this, though I'd like to see it do a few more things (happy to help implement):

  1. Be able to get the Report subclass from a downstream package. I think it'd be useful to have scooby look for a Report class at the top level namespace of a package so that something like scooby --report pyvista would yield pyvista.Report()
  2. Be able to grep for package names so that I could do scooby django-* and get a list of all the django-* packages like django-allauth, etc. This would be analogous to pip list | grep django-
  3. Have scooby execute a Python script/command with tracked imports. This might look like scooby --track foo.py which would use tracking features to output a tracked report after completion.

@prisae
Copy link
Collaborator Author

prisae commented Jul 31, 2022

I implemented your suggestion (1), and simplified the whole CLI to a single file.

You might have to have a look at (2) and (3), as you did the tracker that might be faster. However, we might also just merge what we have, and add additional options to the CLI as we have time.

@prisae prisae mentioned this pull request Oct 17, 2022
@banesullivan banesullivan self-requested a review October 19, 2022 05:53
scooby/__main__.py Outdated Show resolved Hide resolved
@smason
Copy link

smason commented Oct 19, 2022

as another minor comment, main is turning into a bit of a beast. I tend to split the command line parsing out into its own function, e.g.:

def parseargs(argv):
    parser = ArgumentParser()
    # ...
    args = parser.parse_args(argv or sys.argv[1:])
    if args.version:
        print(f"scooby v{scooby.__version__}")
        sys.exit()
    return args

I also tend to use the Namespace object rather than converting to a dict, never seen that, but docs do mention that even if all the examples there leave it in object form.

@prisae
Copy link
Collaborator Author

prisae commented Oct 19, 2022

I agree that __main__ got bigger than intended, because things got added (as always). I usually also use an own parser function. Here, in this case, I don't really mind either way. Same regarding Namespace vs dict.

What are your opinions @banesullivan , @akaszynski ?

@banesullivan
Copy link
Owner

+1 for splitting the command line parsing out into its own function.

Re Namespace vs dict... I'm not sure I follow...

@prisae, I'm happy with this in it's current state, so feel free to merge (and bump the version) when you feel it is ready

@prisae
Copy link
Collaborator Author

prisae commented Oct 28, 2022

I split it into a parser function and a function that acts upon it.

I left it as dict. I am simply used to that way, particularly as I can provide that dict, in some other cases, directly as kwargs to the subsequent functions.

@prisae
Copy link
Collaborator Author

prisae commented Oct 28, 2022

Brining this in, thanks @smason for all the feedback and inputs!

@prisae prisae merged commit 10ce690 into main Oct 28, 2022
@prisae prisae deleted the cli branch October 28, 2022 09:56
@prisae prisae changed the title CLI - initial commit Add CLI Oct 28, 2022
@prisae prisae mentioned this pull request Oct 28, 2022
@prisae
Copy link
Collaborator Author

prisae commented Nov 4, 2022

I forgot to add the entry_point to the conda-forge feedstock, so the CLI won't work on Windows when installed through conda.

conda-forge/scooby-feedstock#22 is fixing this.

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.

4 participants