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

entgql: refactor Skip annotation #268

Merged
merged 13 commits into from
Mar 27, 2022
Merged

entgql: refactor Skip annotation #268

merged 13 commits into from
Mar 27, 2022

Conversation

giautm
Copy link
Collaborator

@giautm giautm commented Mar 24, 2022

@giautm giautm force-pushed the refactor-skip-annotation branch from 437a0b9 to e46c4ca Compare March 24, 2022 18:40
@giautm giautm requested a review from a8m March 24, 2022 18:41
Comment on lines 70 to 78
SkipFlagType SkipFlag = 1 << iota
// SkipFlagFieldEnum will skip generate Enum from the enum field
SkipFlagFieldEnum
// SkipFlagInput will skip mutation input for the entity/field
SkipFlagInput
// SkipFlagWhere will skip input filter for the entity/field
SkipFlagWhere
// SkipFlagOrder will skip generate sort order for the entity
SkipFlagOrder
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SkipFlagType SkipFlag = 1 << iota
// SkipFlagFieldEnum will skip generate Enum from the enum field
SkipFlagFieldEnum
// SkipFlagInput will skip mutation input for the entity/field
SkipFlagInput
// SkipFlagWhere will skip input filter for the entity/field
SkipFlagWhere
// SkipFlagOrder will skip generate sort order for the entity
SkipFlagOrder
SkipNode SkipFlag = 1 << iota
// SkipEnum will skip generate Enum from the enum field
SkipEnumField
// SkipMutationInput will skip mutation input for the entity/field
SkipMutationInput
// SkipWhereInput will skip input filter for the entity/field
SkipWhereInput
// SkipOrderField will skip generate sort order for the entity
SkipOrderField

I'm thinking about how the API will look and I shorter feels cleaner to me. Another point, what does it mean to SkipOrderField or SkipEnumField? Skip generating only GraphQL or also in Ent? Do we need a flag for both options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SkipNode: I thought about this name before, because we need to use it for all fields. So SkipNode does not seem appropriate. Do we need to add SkipField as well? SkipType seems more suitable to me, as it comes with SkipInput

Another point, what does it mean to SkipOrderField or SkipEnumField? Skip generating only GraphQL or also in Ent

It should skip generating both Go code and GQL schema.

Copy link
Member

Choose a reason for hiding this comment

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

SkipType seems more suitable to me, as it comes with SkipInput

Right, let's use SkipType. I wonder how's the usage will look like:

func (T) Annotations() []schema.Annotation {
    return []schema.Annotation{
        // Default to skip All.
        entgql.Skip(),

        // Or, skip only type and input generation.
        entgql.Skip(entgql.SkipType|entgql.SkipInput),
    }
}

func (T) Fields() []ent.Field {
    return []ent.Field{
        field.Int("age").
            Annotations(
                // Skip all.
                entgql.Skip(),

                // Or, skip only from input?
                entgql.Skip(entgql.SkipInput),
            ),
    }
}

This is the API you thought about or you have something else in mind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's almost the same.

func (T) Annotations() []schema.Annotation {
    return []schema.Annotation{
        // Default to skip All.
        entgql.Skip(),

        // Or, skip only type and input generation.
        entgql.Skip(entgql.SkipType, entgql.SkipMutationInput),
    }
}

func (T) Fields() []ent.Field {
    return []ent.Field{
        field.Int("age").
            Annotations(
                // Skip all.
                entgql.Skip(),

                // Or, skip only from input?
                entgql.Skip(entgql.SkipMutationInput),
            ),
    }
}

@giautm giautm force-pushed the refactor-skip-annotation branch from e46c4ca to b76066e Compare March 26, 2022 23:05
Comment on lines 67 to 82
const (
// SkipType will skip generating the entity or field in the schema
SkipType SkipMode = 1 << iota
// SkipEnumField will skip generating the enum type from the enum field
SkipEnumField
// SkipOrderField will skip generating the entity or the field for the enum order
SkipOrderField
// SkipWhereInput will skip generating the entity or the field in the WhereInput
SkipWhereInput

// SkipAll is default mode to skip all
SkipAll = SkipType |
SkipEnumField |
SkipOrderField |
SkipWhereInput
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @a8m, let's review the naming and the description again.

@giautm giautm force-pushed the refactor-skip-annotation branch from ccaaf29 to f9f1ad5 Compare March 27, 2022 05:52
Comment on lines 204 to 208
// Any returns true if the skip annotation is setted
func (f SkipMode) Any() bool {
return f != 0
}
Copy link
Member

Choose a reason for hiding this comment

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

Why it's preferred over != 0?

@@ -71,6 +71,8 @@ var (
"filterNodes": filterNodes,
"findIDType": findIDType,
"nodePaginationNames": nodePaginationNames,
"skipMode": skipModeFromString,
"hasSkipMode": hasSkipMode,
Copy link
Member

Choose a reason for hiding this comment

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

This function is not needed IMO, you can use the skipMode function in the template (0 is falsy).

@@ -152,15 +154,15 @@ func filterNodes(nodes []*gen.Type) ([]*gen.Type, error) {
}

// filterEdges filters out edges that should not be included in the GraphQL schema.
func filterEdges(edges []*gen.Edge) ([]*gen.Edge, error) {
func filterEdges(edges []*gen.Edge, skip SkipMode) ([]*gen.Edge, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Make it an optional param to not break existing templates used by users.

@giautm giautm force-pushed the refactor-skip-annotation branch from 9e0ba20 to c8eb2be Compare March 27, 2022 09:00
@giautm giautm merged commit 0e7eced into master Mar 27, 2022
@giautm giautm deleted the refactor-skip-annotation branch March 27, 2022 09:20
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