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

Fix missing ValidatorMessage stub in Python bindings #65

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Jun 21, 2023

Type of PR:

  • Bugfix

Required reviews:

  • 1

What this does:

  • Replaces tuple usage with a concrete ValidatorMessage type in Python Bindings

Issues fixed/closed:

Why it's needed:

Notes for reviewers:

@codecov-commenter
Copy link

Codecov Report

Merging #65 (704a2ea) into main (830dc68) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #65   +/-   ##
=======================================
  Coverage   19.50%   19.50%           
=======================================
  Files          17       17           
  Lines        3214     3214           
=======================================
  Hits          627      627           
  Misses       2587     2587           

@derekpierre
Copy link
Member

An API is changing, so should the Changelog be updated?

CHANGELOG.md Outdated
@@ -9,9 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Fixed a typo in the Python type stubs for `ferveo.Keypair.secure_randomness_size()`. ([#61])
- Replaced raw tuples with `ValidatorMessage` in Python bindings. ([ferveo#131])
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is a breaking change, i.e. major version change...?
Also, should this just link to this PR (#65) instead of the ferveo link? I think the goal is to link to nucypher-core specific changes.

(^ cc @fjarri )

Copy link
Member

Choose a reason for hiding this comment

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

i.e. major version change...?

I think your changelog entries should be under 0.10.0 because of the breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should be under ### Changed subsection, not ### Fixed.

@piotr-roslaniec piotr-roslaniec force-pushed the ferveo#131 branch 5 times, most recently from 6c5f6cc to 6bea0ba Compare June 22, 2023 08:36
@piotr-roslaniec
Copy link
Contributor Author

Fixed & rebased

@derekpierre
Copy link
Member

Needs another rebase now that #63 has been merged.

@piotr-roslaniec
Copy link
Contributor Author

Rebased again. Added a small, unrelated change related to the previous PR that I didn't catch.

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

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

🎸

@piotr-roslaniec piotr-roslaniec merged commit dc2d3fc into nucypher:main Jun 22, 2023
@derekpierre
Copy link
Member

derekpierre commented Jun 22, 2023

@piotr-roslaniec I thought we weren't merging with github dependencies?

ferveo = { package = "ferveo-pre-release", version = "0.1.0-alpha.8", features = ["bindings-python"], git = "https://github.com/nucypher/ferveo.git", rev = "0d4e973e007b16cff34d649ae107608c809349af" }

@piotr-roslaniec
Copy link
Contributor Author

@derekpierre, Yes, my bad, I will replace it with a new release nucypher/ferveo#132 shortly

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.

Handle downstream changes from ferveo-pre-release@0.1.0-alpha.9
4 participants