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

API: Make _type read-only #1372

Merged
merged 1 commit into from
May 6, 2021
Merged

Conversation

jku
Copy link
Member

@jku jku commented May 4, 2021

Also remove _type from Signed constructor arguments: the value is in a
class attribute. This way _type never needs to be validated (except
in the dispatcher in Metadata). There is a double-check in
_common_fields_from_dict() just to be sure.

This makes the API easier to use correctly as the public property is
immutable.

This is an API break as all Signed constructors change -- this could be
avoided but seems like the correct choice.

Signed-off-by: Jussi Kukkonen [email protected]


Fixes #1371

The discussion-worthy aspects of this PR are:

  • it's a breaking change: It changes all Signed-derived constructors API (although calling Signed constructors isn't a common thing to do -- it usually happens in from_dict())
  • it uses a property to make an attribute read-only: this is not done so far in the API but I believe we should do that more for attributes that should not be changed or that should be changed via some setter function.
  • 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

Also remove _type from Signed constructor arguments: the value is in a
class atttribute. This way _type never needs to be validated (except
in the dispatcher in Metadata). There is a double-check in
_common_fields_from_dict() just to be sure.

This makes the API easier to use correctly as the public property is
immutable.

This is an API break as all Signed constructors change -- this could be
avoided but seems like the correct choice.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku marked this pull request as draft May 4, 2021 07:02
@jku jku marked this pull request as ready for review May 4, 2021 07:07
Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. I like removing _type from the constructor and making the property read-only.

Given that this is an in-flux API, I'm not worried about the API break. Let's get this right before we promise something stable.

@woodruffw is not using the constructors directly in the PyPI/warehouse integration, IIRC. (However I think he already has changes to make as that WIP PR is using Metadata.from_json_file(), which has been renamed Metadata.from_file())

@jku jku merged commit 5c1b12f into theupdateframework:develop May 6, 2021
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.

API: Make _type read-only
2 participants