Skip to content

support feature variant #13

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.

Sign up for GitHub

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

Merged
Eskibear merged 9 commits into from
Aug 15, 2024
Merged

support feature variant #13

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
504b6e0
support feature variant
Eskibear Jul 4, 2024
e51b959
add test cases
Eskibear Jul 31, 2024
3ed3d75
refactor isEnabled to support status_override
Eskibear Jul 31, 2024
71fe5f3
add missing return statement
Eskibear Aug 6, 2024
bf5b0e1
address comments: simplify code
Eskibear Aug 6, 2024
a7f0472
fix range validation: align with .NET
Eskibear Aug 6, 2024
ca66f77
use a single interface for the public APIs
Eskibear Aug 7, 2024
63cdd43
extract common code as getVariantAssignment
Eskibear Aug 8, 2024
422b562
address comments
Eskibear Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add missing return statement
  • Loading branch information
@Eskibear
Eskibear committed Aug 6, 2024
commit 71fe5f3643e18c8233f9a92b9edcf9b1c97ed36b
1 change: 1 addition & 0 deletions src/featureManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export class FeatureManager implements IFeatureManager, IVariantFeatureManager {
return { variant, reason: VariantAssignmentReason.User };
Copy link
Member

@zhiyuanliang-ms zhiyuanliang-ms Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should it be { variant: variant, reason: VariantAssignmentReason.User }?

Copy link
Member Author

@Eskibear Eskibear Aug 5, 2024

Choose a reason for hiding this comment

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

{variant} is short for {variant: variant}. A syntax sugar for simplicity, I can make it clearer if it looks confusing to you now.

Copy link
Member

@zhiyuanliang-ms zhiyuanliang-ms Aug 5, 2024

Choose a reason for hiding this comment

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

I am ok with it. But personally, I prefer consistency. Is there linting rule for this?

Copy link
Member Author

@Eskibear Eskibear Aug 6, 2024

Choose a reason for hiding this comment

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

Of course there is corresponding linting rule, it's called object-shorthand.
This shorthand syntax makes the code more readable and is widely adopted in ES6 and later versions of JavaScript. See the example of this rule, usually we force to use object shorthand in linting rules instead of forbidding.

} else {
console.warn(`Variant ${userAllocation.variant} not found for feature ${featureFlag.id}.`);
return { variant: undefined, reason: VariantAssignmentReason.None };
}
}
}
Expand Down