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

Hub version command cli #628

Merged

Conversation

sparkingdark
Copy link
Contributor

@sparkingdark sparkingdark commented Feb 27, 2021

This will add the version command to hub cli and it can be used like this:

image

fixes issue #627

I think it will be better if I use from setup import VERSION but I got some errors due to packages and python version 3.8.
can you give some highlight regarding this @mynameisvinn @AbhinavTuli

@github-actions
Copy link

Locust summary

Git references

Initial: 1ac80bb
Terminal: d9fb971

hub/cli/auth.py
Changes:
  • Name: register
    Type: function
    Changed lines: 1
    Total lines: 17
    hub/cli/command.py
    Changes:
    • Name: version
      Type: function
      Changed lines: 3
      Total lines: 3
      • Name: add_commands
        Type: function
        Changed lines: 1
        Total lines: 5

        @click.command()
        def version(version=1.22):
        """Get the version of the hub package"""
        click.echo(f"you are using the hub version:{version}")
        Copy link
        Contributor

        Choose a reason for hiding this comment

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

        Thanks for adding this - we definitely need to be able to retrieve the version from the CLI.
        I would suggest storing a version in one central place (currently we keep it in setup.py but probably version would be better) and then importing it in the CLI rather then setting it there. Also, it's better to return just the version or package name (Hub) + the version. Perhaps, the version should be just an argument to hub, not a separate command. In that case, we could invoke it in the following way: hub --version.

        Compare python --version.

        Copy link
        Contributor Author

        Choose a reason for hiding this comment

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

        Yeah tried to do but got some distribution error regarding some packages, so is it okay to store it in version.py and import as required. Actually I don't understand how to add --version in the click module, can you give some hint.

        Copy link
        Contributor Author

        Choose a reason for hiding this comment

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

        Added requested changes @mynameisvinn @haiyangdeperci

        @mynameisvinn mynameisvinn added enhancement New feature or request help wanted Extra attention is needed labels Feb 27, 2021
        @mynameisvinn mynameisvinn self-requested a review February 27, 2021 13:18
        @sparkingdark
        Copy link
        Contributor Author

        Now it is look like this
        image

        Copy link
        Contributor

        @haiyangdeperci haiyangdeperci left a comment

        Choose a reason for hiding this comment

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

        This is all right now. One possible enhancement could be swapping the version in setup.py to have a single source of truth for the version.

        @codecov
        Copy link

        codecov bot commented Feb 27, 2021

        Codecov Report

        Merging #628 (ca5c836) into master (1ac80bb) will decrease coverage by 0.20%.
        The diff coverage is 10.00%.

        Impacted file tree graph

        @@            Coverage Diff             @@
        ##           master     #628      +/-   ##
        ==========================================
        - Coverage   89.32%   89.12%   -0.21%     
        ==========================================
          Files          54       55       +1     
          Lines        3925     3935      +10     
        ==========================================
        + Hits         3506     3507       +1     
        - Misses        419      428       +9     
        Impacted Files Coverage Δ
        hub/cli/command.py 0.00% <0.00%> (ø)
        hub/version.py 0.00% <0.00%> (ø)
        hub/cli/auth.py 53.57% <100.00%> (+0.84%) ⬆️

        Continue to review full report at Codecov.

        Legend - Click here to learn more
        Δ = absolute <relative> (impact), ø = not affected, ? = missing data
        Powered by Codecov. Last update 1ac80bb...7d03cf6. Read the comment docs.

        hub/cli/auth.py Outdated Show resolved Hide resolved
        @sparkingdark
        Copy link
        Contributor Author

        sparkingdark commented Feb 28, 2021 via email

        @haiyangdeperci
        Copy link
        Contributor

        haiyangdeperci commented Mar 1, 2021

        @mynameisvinn Could you take a look?

        What's the reasoning behind separate CLI version?

        1.16.39 Outdated Show resolved Hide resolved
        hub/cli/auth.py Outdated Show resolved Hide resolved
        @haiyangdeperci haiyangdeperci self-requested a review March 1, 2021 08:55
        @sparkingdark
        Copy link
        Contributor Author

        can you check @haiyangdeperci @mynameisvinn

        @AbhinavTuli
        Copy link
        Contributor

        I agree with @haiyangdeperci, version info should be stored only in a single place and accessed by both setup.py and this click command.

        @sparkingdark
        Copy link
        Contributor Author

        @AbhinavTuli I try to import version from the setup.py but got error regarding some distributions in package.

        @haiyangdeperci
        Copy link
        Contributor

        I think @AbhinavTuli meant that you import the version in setup.py

        @mynameisvinn
        Copy link
        Contributor

        @mynameisvinn Could you take a look?

        What's the reasoning behind separate CLI version?

        @haiyangdeperci Not sure since this predates me. We should remove it.

        @mynameisvinn mynameisvinn merged commit e890baf into activeloopai:master Mar 2, 2021
        @mynameisvinn
        Copy link
        Contributor

        This looks good @sparkingdark! Nice work!

        @sparkingdark
        Copy link
        Contributor Author

        Thanks @mynameisvinn

        @sparkingdark sparkingdark deleted the hub_version_command_cli branch March 2, 2021 02:28
        Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
        Labels
        enhancement New feature or request help wanted Extra attention is needed
        Projects
        None yet
        Development

        Successfully merging this pull request may close these issues.

        None yet

        4 participants