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

ngclient: review argument names #1638

Closed
jku opened this issue Oct 27, 2021 · 3 comments · Fixed by #1731
Closed

ngclient: review argument names #1638

jku opened this issue Oct 27, 2021 · 3 comments · Fixed by #1731
Assignees
Labels
backlog Issues to address with priority for current development goals ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented Oct 27, 2021

Now that API seems to stabilizing (and before we have a stable release) let's check that argument names ngclient uses are consistent.

Lukas in #1604:

The pairs repository_dir - metadata_base_url, and target_dir - target_base_url both describe the local and remote location respectively for the given file type. Maybe we could use names that show this duality more directly? Here are some naming suggestions

  1. Rename repository_dir to metadata_dir. IMO repository_dir sounds more like something that contains metadata, targets and keys (at least that was the case in the old implementation), whereas here it only contains metadata.
  2. Use remote and local prefixes, e.g. local_metadata_dir and remote_metadata_base_url or, to make the latter shorter, remote_metadata_dir?

At least metadata vs repository seems like an obvious flaw: no need to use two different terms.

The second one I'm not so sure about: the names are long enough that I already avoid naming the args because of it, and for me "url" vs "dir" does give a hint that one is remote and one is not remote...

@jku jku mentioned this issue Oct 27, 2021
3 tasks
@jku jku added the ngclient label Oct 27, 2021
@lukpueh
Copy link
Member

lukpueh commented Oct 27, 2021

... "url" vs "dir" does give a hint that one is remote and one is not remote

I did have a similar hunch. Just wanted to briefly rethink this together. But I'm fine with leaving those and don't make the names any longer.

@lukpueh
Copy link
Member

lukpueh commented Oct 27, 2021

Since we have this issue, I'll bring up another very mild naming concern I had.

When I read target_dir, I inevitably think of the man pages for ln or cp and that something is moved somewhere. There's probably not much we can do, but would targets_dir (plural) make it a little better? Or will that make it look like a directory for targets metadata? Not trying to bikeshed here, and happy to let it go if nobody thinks this is an issue or has a better idea.

@sechkova sechkova added the backlog Issues to address with priority for current development goals label Oct 27, 2021
@sechkova sechkova added this to the Sprint 14 milestone Dec 8, 2021
@jku
Copy link
Member Author

jku commented Dec 8, 2021

Argument names currently

Constructor: 
    repository_dir
    metadata_base_url
    target_dir
    target_base_url

get_targetinfo()
    target_path

find_cached_target()
    targetinfo
    filepath

download_target()
    targetinfo
    filepath
    target_base_url 

possible changes:

  • make repository_dir clearer: rename to metadata_dir
  • make target_dir clearer: maybe rename to targets_dir or download_dir? same rename should probably be done to target_base_url
  • maybe filepath should be better tied to the targets directory name: if we have download_dir we could also rename filepath to download_path?

jku pushed a commit to jku/python-tuf that referenced this issue Dec 15, 2021
metadata_dir matches metadata_base_url better.

This is an API break for anyone using named arguments.

Fixes theupdateframework#1638

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants