Skip to content

Introduce PipeSeparatedFlags<T> marker class #2214

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

Merged
merged 2 commits into from
Aug 15, 2023
Merged

Introduce PipeSeparatedFlags<T> marker class #2214

merged 2 commits into from
Aug 15, 2023

Conversation

flobernd
Copy link
Member

@flobernd flobernd commented Aug 1, 2023

What?

This PR introduces the FlagsEnum<T> marker class similar to the Stringified<T> concept.

Flags enums are defined as enums with a special case serialization. Values of this enum are represented as string (like for regular enums as well). In addition, individual enum member values can be combined using the pipe separator char (|) like e.g. described here:
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-simple-query-string-query.html#supported-flags

Why?

At the moment, this specific construct is defined in 2 parts:

  1. The enum itself (TEnum) as a regular enum type
  2. A type alias defined as FlagsEnum = TEnum | string

Using just a generic union makes it hard to generate correct (de-)serialization code and "hard" for the users to use properties of this type, as they have to manually craft strings when using multiple values (unintuitive, error prone, ...).

In case of C#, this marker would allow me to e.g. generate a native language construct:

[Flags]
public enum MyEnum
{
    None = 0,
    A = 1 << 0,
    B = 1 << 1,
    ...
}

and allows the user to utilize the standard way of working with flags:

var flags = MyEnum.A | MyEnum.B;
if (flags.HasFlag(MyEnum.B))
{
    ...
}

@flobernd flobernd requested a review from a team as a code owner August 1, 2023 12:16
*/
export type SimpleQueryStringFlags = SimpleQueryStringFlag | string
export type SimpleQueryStringFlags = FlagsEnum<SimpleQueryStringFlag[]>

export enum SimpleQueryStringFlag {
Copy link
Member Author

Choose a reason for hiding this comment

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

Does not make a real difference as these values are currently not part of the generated schema, but I adjusted them anyways based on their actual values:

https://lucene.apache.org/core/8_9_0/queryparser/constant-values.html#org.apache.lucene.queryparser.simple.SimpleQueryParser.AND_OPERATOR

Copy link
Member

Choose a reason for hiding this comment

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

Actually you can remove these constants altogether. They're some ancient leftovers from the very first iterations on the TypeScript spec where integer constants were added to the enums.

@flobernd flobernd requested a review from swallez August 1, 2023 12:26
@flobernd flobernd changed the title Introduce FlagsEnum<T> marker class Introduce PipeSeparatedFlags<T> marker class Aug 14, 2023
Copy link
Member

@swallez swallez left a comment

Choose a reason for hiding this comment

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

LGTM. As mentioned in the comment, we can remove the integer constants in the enum.

*/
export type SimpleQueryStringFlags = SimpleQueryStringFlag | string
export type SimpleQueryStringFlags = FlagsEnum<SimpleQueryStringFlag[]>

export enum SimpleQueryStringFlag {
Copy link
Member

Choose a reason for hiding this comment

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

Actually you can remove these constants altogether. They're some ancient leftovers from the very first iterations on the TypeScript spec where integer constants were added to the enums.

@flobernd flobernd merged commit bbb91fe into main Aug 15, 2023
@flobernd flobernd deleted the flags-enum branch August 15, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants