Skip to content

Experimental client: renaming #1401

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

Merged
merged 5 commits into from
May 21, 2021

Conversation

sechkova
Copy link
Contributor

@sechkova sechkova commented May 20, 2021

Fixes #1397
Description of the changes being introduced by the pull request:

Note: this is done on top of changes introduced by #1396

Proposes the following client naming and directory structure:

tuf/
    ngclient/
        __init__.py
        updater.py
        metadatabundle.py
        network/
            __init__.py
            requestsfetcher.py
            download.py

Most significant changes:

  • client_rework -> ngclient (next-gen-client): almost what @jku suggested but trying to avoid dashes (not accepted) and underscores (not recommended) in package names.
  • client public classes are exposed in ngclient/__init__.py
  • download related modules are moved under network. FetcherInterface is defined in network/__init__.py instead of fetcher.py
  • new client code is excluded from coverage until better tested

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@sechkova sechkova requested a review from jku May 20, 2021 11:48
@sechkova sechkova added the experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch) label May 20, 2021
@jku
Copy link
Member

jku commented May 20, 2021

Note that the names here are not package names: they're just module names where underscores can be used if it improves readability... but I'm fine with ngclient, no better ideas at the moment

@jku
Copy link
Member

jku commented May 20, 2021

The reason I suggested ngclient._internal is that the name makes it obvious to an outsider that they should not rely on things inside this module -- we want to be able to change them when we want. ngclient.network does not convey that and I don't think we can assume outsiders understand that ngclient (the init.py) defines what our API is and that tings below it are off-limits (as much as I otherwise like the concept).

@jku
Copy link
Member

jku commented May 20, 2021

Note that the names here are not package names: they're just module names where underscores can be used if it improves readability... but I'm fine with ngclient, no better ideas at the moment

I'll just correct myself here: they do seem to call any directory with __init__.py a package.

I still maintain _internal is a fine name for a directory: this what a lot of big name python projects do (see pip for an example).

@sechkova
Copy link
Contributor Author

Agreed with @jku's review to move non-public modules to _internal.
The new structure is:

tuf/
    ngclient/
        __init__.py
        updater.py
        fetcher.py
        _internal/
            requests_fetcher.py
            download.py
            metadata_bundle.py

Also rebased on latest changes in experimental-client branch.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Thanks!

There's obviously many valid choices here but I'm happy with ones made here

@jku
Copy link
Member

jku commented May 21, 2021

For some reason github claims there are conflicts here after merging 1403 -- manual rebase works without any issues though?

sechkova added 5 commits May 21, 2021 10:00
The current client and the next-gen client should
coexist in the same repository during the ongoing
development of the latter.

Looking for a name which is client-related,
short, meeting PEP8 package names requirements.
Currently "ngclient" seems to fit in until a
better proposal comes.

Rename updater_rework.py to updater.py

Signed-off-by: Teodora Sechkova <[email protected]>
Separate public/private API. Keep modules
containing the piblic classes in the main client
directory and move the rest to _internal.

Signed-off-by: Teodora Sechkova <[email protected]>
Only "Updater" and "FetcherInterface" are considered
public classes of the client. Exposing them in __init__.py
makes usage and access simpler.

Signed-off-by: Teodora Sechkova <[email protected]>
Use the same call of black, isort, pylint to cover
multiple directories.

Signed-off-by: Teodora Sechkova <[email protected]>
Restore coverage back to 97% but omit ngclient
form the overall score until tests are implemented.

Signed-off-by: Teodora Sechkova <[email protected]>
@sechkova
Copy link
Contributor Author

Yep, rebased.

@sechkova sechkova changed the base branch from experimental-client to develop May 21, 2021 07:34
@sechkova sechkova changed the base branch from develop to experimental-client May 21, 2021 07:38
@jku jku merged commit 5f37eb3 into theupdateframework:experimental-client May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants