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

Remove "legacy code" #1745

Closed
lukpueh opened this issue Dec 22, 2021 · 5 comments · Fixed by #1790
Closed

Remove "legacy code" #1745

lukpueh opened this issue Dec 22, 2021 · 5 comments · Fixed by #1790
Assignees
Labels
1.0.0 blockers To be addressed before 1.0.0 release
Milestone

Comments

@lukpueh
Copy link
Member

lukpueh commented Dec 22, 2021

The recently released v0.20.0 was the last release to include the legacy implementation. In preparation for v1.0.0 it is time to actually say farewell to the legacy implementation. Here is a (likely non-exhaustive) list of related tasks:

  • Identify and remove legacy code, tests and documentation
  • Review and update continued resources as necessary
  • Consider renames
    • ngclient -> client
      IMO the name ngclient makes most sense in relation to the "old" client and so will lose its meaning after the old client has gone, at least after a while
    • api.metadata -> metadata
      IIRC the api directory was a bit of a crutch to develop new code alongside legacy code without mixing them up, which we won't need after removing the legacy code. IMO api should be designated by other means, e.g. on RTD
@jku
Copy link
Member

jku commented Jan 7, 2022

ngclient -> client

I prefer keeping ngclient: this way any old code from tuf.client.updater import Updater fails immediately which I think is a good thing now that the API is slightly different. I don't think client is significantly more discoverable either: users are going to read the API reference or example code anyway.

api.metadata -> metadata

Fine by me (although I don't feel a strong need: imports look fine to me now). If we do this I would suggest we consider keeping the code where it is though (for git reasons) and solving the problem with clever imports.

@MVrachev
Copy link
Collaborator

MVrachev commented Jan 7, 2022

  • Consider renames

    • ngclient -> client
      IMO the name ngclient makes most sense in relation to the "old" client and so will lose its meaning after the old client has gone, at least after a while
    • api.metadata -> metadata
      IIRC the api directory was a bit of a crutch to develop new code alongside legacy code without mixing them up, which we won't need after removing the legacy code. IMO api should be designated by other means, e.g. on RTD

Are we sure we won't lose the Git history if we do this?
If we do, then I will prefer if we don't rename...
See my comment #1582 (comment).

@lukpueh
Copy link
Member Author

lukpueh commented Jan 11, 2022

I don't think client is significantly more discoverable either: users are going to read the API reference or example code anyway.

To me it's not about discoverability, but more about the baggage of the "previous generation"-client the name refers to, which I think is unnecessary.

The "error on import argument" is fair, although I don't think this will be a problem in practice.

I would suggest we consider keeping the code where it is though (for git reasons) and solving the problem with clever imports

Isn't part of the python zen to not be clever with imports? :) Also, an import alias wouldn't really solve the problem that the repo has an unnecessary subfolder.

So I'd either rename or leave as is. And I don't think git history is a problem, because many git features work across renames out of the box, and those that don't can usually be worked around when you know that there was a rename at some point, which can be communicated in the renaming commit's message.

@jku jku added the 1.0.0 blockers To be addressed before 1.0.0 release label Jan 12, 2022
@jku
Copy link
Member

jku commented Jan 12, 2022

Isn't part of the python zen to not be clever with imports

You make a valid point ... but as an example of something not-too-clever I think we aren't using __init__.py like we could -- so we could make from tuf.api.metadata import Metadata into from tuf.api import Metadata if we wanted and I think that would make the import less ugly? So then the only issue would be that we don't have a "main" package -- which is fine to me: I don't think we should necessarily be advertizing Metadata API as the main thing... ngclient and future repo tooling might be the more user facing features anyway

@lukpueh lukpueh self-assigned this Jan 12, 2022
@MVrachev
Copy link
Collaborator

  • Consider renames

After a meeting with @jku and @lukpueh it was decided that there is no need to do renames.

  • ngclient -> client
    IMO the name ngclient makes most sense in relation to the "old" client and so will lose its meaning after the old client has gone, at least after a while

Using ngclient as opposed to the client has the advantage that when an user of the old code wants to integrate to the new client the imports will fail and this will lead him/her to read our RTD documentation.
Otherwise, if we use the client name the import will succeed and the API itself will fail at a later stage which will be strange.

  • api.metadata -> metadata
    IIRC the api directory was a bit of a crutch to develop new code alongside legacy code without mixing them up, which we won't need after removing the legacy code. IMO api should be designated by other means, e.g. on RTD

There were no strong preferences between using using api or something else. We all agreed that renaming it to metadata specifically was a bad idea given the use case when the user wants do import Metadata from tuf.metadata.metadata which sounds... weird.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0 blockers To be addressed before 1.0.0 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants