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

refactor(pass-style): Make @fast-check/ava a devDependency #2645

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

gibson042
Copy link
Contributor

@gibson042 gibson042 commented Nov 22, 2024

Fixes #1741

Caution

While technically a breaking change, it is pragmatically not marked as such because we are not aware of anything outside of Endo that imports a fast-check arbitrary.

Description

Rather than importing fc directly to export arbitraries, @endo/pass-style/tools.js now returns them from an exported makeArbitraries function that requires fc as an argument.

Best reviewed with whitespace ignored.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

n/a

Compatibility Considerations

This is a breaking change, but only to @endo/pass-style/tools.js. All imports in this repository have been updated, and I didn't see any in agoric-sdk or agoric-3-proposals, but if there are any then they will need similar updates.

Upgrade Considerations

This only affects code intended for testing, and I'm disinclined to even mention it in NEWS.md.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

@erights
Copy link
Contributor

erights commented Nov 22, 2024

Note to other reviewers: best to review diffs after hiding whitespace diffs

@dckc
Copy link
Contributor

dckc commented Nov 22, 2024

Will the breaking change major version bump ripple thru the rest of endo packages?

@kriskowal does this seem appropriate to you? We agonized about a similar situation not long ago. I don't recall clearly how similar.

@kriskowal
Copy link
Member

This technically means that the devDependency should become a peerDependency, the effect of which is that the package manager will not install a different version than the parent node_modules if there is a conflict, and won’t install it at all if the parent doesn’t need it, but will balk if the versions are incompatible.

I can be assuaged about the compatibility break and even downgrade the ! conventional commit if you have a #endo-branch integration PR in Agoric SDK that passes tests.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

integration pr plz

Fixes #1741

Rather than importing `fc` directly to export arbitraries,
@endo/pass-style/tools.js now returns them from an exported
`makeArbitraries` function that requires `fc` as an argument.

While technically a breaking change, it is pragmatically not marked as
such because we are not aware of anything outside of Endo that imports
a fast-check arbitrary.
@gibson042 gibson042 force-pushed the gibson-1741-fast-check-ava-dependency branch from ef6e56a to 31864d4 Compare December 2, 2024 21:43
@gibson042 gibson042 changed the title refactor(pass-style)!: Make @fast-check/ava a devDependency refactor(pass-style): Make @fast-check/ava a devDependency Dec 2, 2024
@gibson042
Copy link
Contributor Author

Updated the commit and PR description to remove the breaking-change indication.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

satisfied. thank you.

for the record, was green in CI with integration with agoric-sdk Agoric/agoric-sdk#10608

@gibson042 gibson042 merged commit 882e912 into master Dec 3, 2024
15 checks passed
@gibson042 gibson042 deleted the gibson-1741-fast-check-ava-dependency branch December 3, 2024 18:40
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.

@endo/far depends on @fast-check/ava via pass-style
4 participants