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

feat: add new constructors #49

Merged
merged 11 commits into from
Jan 23, 2025
Merged

feat: add new constructors #49

merged 11 commits into from
Jan 23, 2025

Conversation

storopoli
Copy link
Member

@storopoli storopoli commented Jan 22, 2025

Description

  • Adds new new_* constructors from the different payload types.
  • Adds a warning on raw bytes/vec constructors.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

Closes #48.

src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
src/descriptor.rs Outdated Show resolved Hide resolved
@AaronFeickert
Copy link
Contributor

Is the use of array references (rather than slices) in the constructors unnecessarily limiting for the caller?

@storopoli
Copy link
Member Author

storopoli commented Jan 22, 2025

Is the use of array references (rather than slices) in the constructors unnecessarily limiting for the caller?

I don't think so, because these functions are called with the assumption that the caller knows what the payload is, hence, the array size is known (or should be checked at compile time).
If you don't know what the payload is, then from_bytes/from_vec is the way to go.
That's why we have a "whenever possible" in

/// Users are advised to use the `new_*` methods whenever possible.

EDIT: Also taking slices would mean that all these easy constructors (apart from OP_RETURN) would need to return Result<Descriptor, Err>.

@AaronFeickert
Copy link
Contributor

Understood. I was thinking of it more from the caller's perspective, where it would undoubtedly like to avoid an extra copy if it doesn't have the input value as an array already.

Copy link
Contributor

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

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

One last nit.

src/descriptor.rs Outdated Show resolved Hide resolved
@storopoli storopoli force-pushed the storopoli/constructors branch from 4044d26 to d5daf8f Compare January 22, 2025 21:03
@storopoli
Copy link
Member Author

Gonna leave this until tomorrow so that @bewakes can also take a look if he wants to.

Copy link
Contributor

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

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

LGTM. Tests would be a good idea for a future PR as well.

@storopoli
Copy link
Member Author

LGTM. Tests would be a good idea for a future PR as well.

They are in the docstrings with the hidden assert!s

Copy link
Contributor

@bewakes bewakes left a comment

Choose a reason for hiding this comment

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

Could we add tiny unit tests for these? If you think it's too trivial then its fine but it would really be great, for example we might be constructing a different variant than the constructor method intends to.

@bewakes
Copy link
Contributor

bewakes commented Jan 23, 2025

They are in the docstrings with the hidden assert!

Oops, just saw this.

@storopoli storopoli merged commit 9344124 into main Jan 23, 2025
11 checks passed
@storopoli storopoli deleted the storopoli/constructors branch January 23, 2025 11:12
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.

Add convenient constructors to Descriptor
3 participants