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

Initial feature flags API #274

Draft
wants to merge 2 commits into
base: 1.19.3
Choose a base branch
from
Draft

Conversation

Platymemo
Copy link
Contributor

An API to make use of the new FeatureFlags system.

Originally limited to 64 flags, this API extends the size of feature flags using a BitSet, which grows as needed.

Additionally, the GatedFeature#FILTERED_REGISTRIES set is made mutable to allow for new registries to be added.

@Unique
private final BitSet quilt$additionalFlags = new BitSet();

@Inject(method = "isIn", at = @At(value = "RETURN", ordinal = 2), cancellable = true)
Copy link
Member

@TheGlitch76 TheGlitch76 Mar 1, 2023

Choose a reason for hiding this comment

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

I'd rather see use of @Slice over ordinals. Ordinals are super fragile (what if the method adds an early return at the head?) and if this code ever breaks, it's not going to be clear to me (or whoever else takes up the responsibility of porting) what you were targeting. Feel free to ping me if you need help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, ordinal is fairly safe because the function is 6 lines, an if-else chain with two early returns, then a simple bit mask check. If a slice is still recommended I can put one in, but this is one of the few cases where I think including a slice is unnecessary or possibly even more confusing.

Copy link
Member

Choose a reason for hiding this comment

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

If possible, I'd like enough description of some kind (either a few words in a comment or a slice) that gives me enough of an idea that I could fix the ordinals on this mixin in an update without needing to open up an old QSL workspace and reverse-engineer the injection points myself.

@Platymemo Platymemo marked this pull request as draft March 1, 2023 23:16
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

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

Can't do a proper review just yet (phone review time) so here's a nitpick

* Provides helper methods for working with {@link FeatureFlag}s.
*/
@ApiStatus.NonExtendable
public interface QuiltFeatureFlags {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a (final) class for consistency.

@ix0rai ix0rai added new: module A pull request which adds a new module library: data Related to the data library. t: new api This adds a new API. s: wip This pull request is being worked on. labels Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library: data Related to the data library. new: module A pull request which adds a new module s: wip This pull request is being worked on. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants