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(exo): reform exo amplifier API #1924

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 3, 2024

closes: #XXXX
refs: #1902

Description

At #1902 (comment) @mhofman rightly points out

In the kernel meeting a couple weeks ago, we discussed having the shape of this rights amplification function be a simple function that accepts a facet, and returns the cohort. I think I preferred that shape vs taking a facet name, but I do realize I didn't capture that feedback in this pull request.

This PR fixes that oversight.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

I flagged this PR with a fix(exo)!: ... because this fix is not a compat change from #1902. OTOH, it is unlikely that anyone is using the #1902 API yet, in which case this PR is a compat change from the prior endo state. The last endo release was before #1902, so this PR is also a compat change from the last release.

Should I downgrade to fix(exo): ... ?

@mhofman
Copy link
Contributor

mhofman commented Jan 3, 2024

Should I downgrade to fix(exo): ...

IMO Yes

Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Besides some typing and comment nits, LGTM. Please downgrade the "breaking" conventional commit down to a regular fix given that no release has been made since the feature was introduced.

packages/exo/src/exo-makers.js Show resolved Hide resolved
packages/exo/src/exo-makers.js Outdated Show resolved Hide resolved
packages/exo/src/exo-makers.js Outdated Show resolved Hide resolved
packages/exo/src/exo-makers.js Outdated Show resolved Hide resolved
@erights erights changed the title fix(exo)!: reform exo amplifier API fix(exo): reform exo amplifier API Jan 3, 2024
@erights erights force-pushed the markm-revise-amplifier-api branch from a6f2c3a to 3149e5d Compare January 3, 2024 19:18
@erights
Copy link
Contributor Author

erights commented Jan 3, 2024

Besides some typing and comment nits, LGTM. Please downgrade the "breaking" conventional commit down to a regular fix given that no release has been made since the feature was introduced.

Done

@erights erights enabled auto-merge (squash) January 3, 2024 20:03
@erights erights merged commit 4c67fe2 into master Jan 3, 2024
14 checks passed
@erights erights deleted the markm-revise-amplifier-api branch January 3, 2024 20:08
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.

2 participants