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

Pointer option #24

Closed
wants to merge 3 commits into from
Closed

Conversation

damour
Copy link
Contributor

@damour damour commented Jan 29, 2019

For now there is no way to return slice of pointers to value (common case in grpc for repeated fields).
Proposed add new -pointer flag that can works with/without -slice option.
This is backward incompatible change.

@codecov
Copy link

codecov bot commented Jan 29, 2019

Codecov Report

Merging #24 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #24   +/-   ##
=======================================
  Coverage   81.91%   81.91%           
=======================================
  Files           2        2           
  Lines          94       94           
=======================================
  Hits           77       77           
  Misses         16       16           
  Partials        1        1
Impacted Files Coverage Δ
example/user.go 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce6bd88...3b57df7. Read the comment docs.

@vektah
Copy link
Owner

vektah commented Feb 7, 2019

Can we preserve BC by defaulting is pointer to is slice?

@vektah
Copy link
Owner

vektah commented Feb 7, 2019

If we need a bc break it might be time to parse a type string a little better, eg -type []*foo.User

@damour
Copy link
Contributor Author

damour commented Feb 7, 2019

Can we preserve BC by defaulting is pointer to is slice?

Yes. To preserve BC we need something like this:

  1. if slice=false default pointer=true
  2. if slice=true default pointer=false

IMHO this approach is not looks consistent and intuitive for users.

If we need a bc break it might be time to parse a type string a little better, eg -type []*foo.User

Good suggestion! I'll remove slice flag from https://github.com/vektah/dataloaden/blob/master/dataloaden.go#L13 and add type parsing.

@damour
Copy link
Contributor Author

damour commented Feb 7, 2019

@vektah done, PTAL.

Copy link
Owner

@vektah vektah left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to review this.

Its a step in the right direction, I think we can probably leave it as a string though (its valid go code)

type ValueType struct {
    Name string
    ImportPath string
    Prefix string
}

where prefix is captured by a simple repeating regex `^([]|*)*$``

}

func NewValueTypeFromString(typeName string) (*ValueType, error) {
valueTypeRegexp := regexp.MustCompile(`^(\[\])?(\*)?(.+)+\.(\w+)+$`)
Copy link
Owner

Choose a reason for hiding this comment

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

what about [][]byte?
or []*[]*string

prefix := "*"
if slice {
prefix := ""
if valueType.IsSlice {
prefix = "[]"
data.LoaderName = name + "SliceLoader"
Copy link
Owner

Choose a reason for hiding this comment

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

Name generation probably also needs to be updated to handle these new cases, or let the user specify a name in case there are collisions.

@frederikhors
Copy link

I need this too.

I would love to help in development but as I am still learning Go I don't know how. You are doing a great job.

@vektah vektah mentioned this pull request May 15, 2019
@vektah
Copy link
Owner

vektah commented May 15, 2019

Closing in favor of #28

@vektah vektah closed this May 15, 2019
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.

3 participants