Skip to content

Conversation

@ashtonian
Copy link
Contributor

This adds functions to expose the internal fields object and to create a new parser from a slice of field objects. This lets the user dynamically configure a parser, my use case is to change rql config based on permissions. I would also like to expose the parsed data and related field information to be able to document related metadata for the user like what are available operations. I think it pairs well with #51.

I think it would be nice to refactor to the following, but this is breaking:

func NewParser(c Config, Model interface{}){}
func NewParserF(c Config,  fields []*Fields){}

Added:

func NewParserF(c Config, fields []*Field) (*Parser, error) {}
func (p *Parser) GetFields() []*Field {}

FilterOps map[string]bool
}
type Field struct {
*FieldMeta
Copy link
Owner

Choose a reason for hiding this comment

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

Why not inline its fields inside the Field type?

Comment on lines +220 to +226
func (p *Parser) GetFields() []*Field {
fields := make([]*Field, 0, len(p.fields))
for _, v := range p.fields {
fields = append(fields, v)
}
return fields
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
func (p *Parser) GetFields() []*Field {
fields := make([]*Field, 0, len(p.fields))
for _, v := range p.fields {
fields = append(fields, v)
}
return fields
}
func (p *Parser) Fields() []*Field {
fields := make([]*Field, 0, len(p.fields))
for _, v := range p.fields {
fields = append(fields, v)
}
return fields
}

Why this method is needed? If it's for testing, let's use the existing public API instead.

}

// Does not use config.Model, gets config from Fields)
func NewParserF(c Config, fields []*Field) (*Parser, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Rename to: NewFieldsParser or add a new option as Config.Fields.

Also, an error should return if c.Model was set as the two are mutually exclusive.

@a8m
Copy link
Owner

a8m commented Aug 17, 2023

Thanks for the contribution, @ashtonian. It's appreciated.

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