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

Add support for variable-width bytes in the IR #4097

Merged
merged 9 commits into from
Sep 5, 2024

Conversation

tybug
Copy link
Member

@tybug tybug commented Sep 2, 2024

Many changes here were analogous to strings, and I've unified approaches where it made sense.

@tybug
Copy link
Member Author

tybug commented Sep 2, 2024

I forgot to mention that unfortunately this is a breaking change for alternative backends/providers, as we're changing the provider interface. We could maybe add a dynamic overload based on kwargs on our end if we think it's worthwhile? We'll have to contend with this again when/if we add width to draw_float, which is maybe a good argument for adding it to the interface now with a constant value of 64.

(cc @pschanely: this pr changes draw_bytes from size: int to min_size: int, max_size: Optional[int]).

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Breaking changes are entirely fine at this point; backends are explicitly unstable and we've warned known downstream both generally in advance and now about this PR. (feels polite to make it a minor release though)

Overall looks great; I'd expect to merge with only minor changes.

@tybug
Copy link
Member Author

tybug commented Sep 5, 2024

Breaking changes are entirely fine at this point

That's what I was hoping to hear 😄. Agree with changing to a minor release 👍. Will return to this pull within a few days.

@tybug
Copy link
Member Author

tybug commented Sep 5, 2024

(a few days...or now 🙂)

@pschanely the interface is now:

def draw_string(
    self,
    intervals: IntervalSet,
    *,
    min_size: int = 0,
    max_size: int = COLLECTION_DEFAULT_MAX_SIZE,
    forced: Optional[str] = None,
    fake_forced: bool = False,
) -> str:

def draw_bytes(
    self,
    min_size: int = 0,
    max_size: int = COLLECTION_DEFAULT_MAX_SIZE,
    *,
    forced: Optional[bytes] = None,
    fake_forced: bool = False,
) -> bytes:

noting that draw_string no longer accepts max_size: int | None.

Of course, you're free to change the default for max_size here – otherwise, you can access it at from hypothesis.internal.conjecture.data import COLLECTION_DEFAULT_MAX_SIZE.

@pschanely
Copy link
Contributor

pschanely commented Sep 5, 2024

(a few days...or now 🙂)

@pschanely the interface is now: [...]

Acknowledged!

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks sweet, let's ship it!

@Zac-HD Zac-HD merged commit 342017a into HypothesisWorks:master Sep 5, 2024
58 checks passed
@tybug tybug deleted the draw-bytes-min-max-size-2 branch September 5, 2024 19:53
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.

3 participants