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

Rename generic publickey import function #285

Merged

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Oct 13, 2020

Fixes: # -

Description of the changes being introduced by the pull request:

Rename "public_key" to "publickey" for consistency with other interface key import functions.

This commit also fixes a typo in an error message, removes unnecessary module paths and adds a note to the function docstring to inform about the shortcoming of the generic function.

Note: We might want to revisit said shortcoming in a future interface enhancing PR (also see #251). For now we primarily want to sort out in-toto/in-toto#80, of which this PR is a part.

Please verify and check that the pull request fulfils 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

Rename "_public_key_" to "_publickey_" for consistency with other
interface key import functions.

This commit also fixes a typo in an error message, removes
unnecessary module paths and adds a note to the function docstring
to inform about the shortcoming of the generic function.
@coveralls
Copy link

coveralls commented Oct 13, 2020

Coverage Status

Coverage remained the same at 98.878% when pulling d7dfca6 on lukpueh:rename-import-pubkey into 0ec854b on secure-systems-lab:master.

@SantiagoTorres
Copy link
Collaborator

LGTM. Thanks!

@SantiagoTorres SantiagoTorres merged commit 208ea21 into secure-systems-lab:master Oct 13, 2020
@joshuagl
Copy link
Collaborator

This is a breaking change. Is it worth continuing to provide the old name for a deprecation period?

@SantiagoTorres
Copy link
Collaborator

You mean leave a facade for it for a deprecation period? that sounds sensible. Probably with a printed warning would be ideal.

@trishankatdatadog
Copy link
Contributor

Yeah, there are some nice @deprecated decorators we could use to clearly mark such functions and emit warnings, etc

@joshuagl
Copy link
Collaborator

You mean leave a facade for it for a deprecation period? that sounds sensible. Probably with a printed warning would be ideal.

Yes, that's what I meant. Thanks for clarifying.

As we're building up to major releases of in-toto and tuf I'd like usm as a coalition of projects, to try and improve our maintenance discipline.

@lukpueh
Copy link
Member Author

lukpueh commented Oct 15, 2020

I generally agree, however the function renamed in this PR was only just introduced 2 weeks ago (see #278) and has not been part of any release. So I think we're good with out deprecation strategy in this scenario.

That said, I'm planning to soon submit a PR that does change the interface by adding additional optional arguments that also change the default behavior. I hope you all can weigh in there as well.

@joshuagl
Copy link
Collaborator

I generally agree, however the function renamed in this PR was only just introduced 2 weeks ago (see #278) and has not been part of any release. So I think we're good with out deprecation strategy in this scenario.

Sounds good to me, thanks for providing the extra context.

That said, I'm planning to soon submit a PR that does change the interface by adding additional optional arguments that also change the default behavior. I hope you all can weigh in there as well.

Absolutely.

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.

5 participants