Add support for nested struct array, adds feature in issue #8

[abdulhannanali]

Hi there Authors, Maintainers and Collaborators,

I stumbled upon #8 and saw that it was still open, I am learning golang these days so wanted to learn as well as start contributing to open source. Please accept, in case you think it's a good addition, it was a great learning experience, especially about the reflect library.

This PR features following updates

• Ability to add index within brackets using indexed tag string
• Ability to recurse over the structs within the slices, leading to something like arr[0][a] for a struct that looks like the following

struct {
  arr []struct{ A string }
}

• I am not sure if this ability is provided by rubygems, as I haven't worked with ruby but the test case in Adding support to encoding of nested struct arrays #8 does list this in test case
• Refactoring of the genericTestRunner to use DRY principle
• Refactoring of the tagParser to use constant strings within the encode.go, in order to reduce the chances of future mistakes

Further improvements

I have a few more features in mind and would add these and open issues at your discretion

• Support Multi Dimensional Array query string conversion, it seems rails doesn't provide this ability for now source
• Pre-parse the options of a tag for a micro-optimization purpose, for rest of the encode.go we could use something faster than string comparison maybe, such as iota, I recently stumbled on that
• Benchmark comparisons with rest of the libraries

Thank you for the library, It's amazing!

[google-cla]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[codecov]

Codecov Report

Merging #48 (e998a99) into master (1f4a1f9) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #48   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          122       139   +17     
=========================================
+ Hits           122       139   +17     

Impacted Files	Coverage Δ
query/encode.go	100.00% <100.00%> (ø)

---

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 1f4a1f9...e998a99. Read the comment docs.

[abdulhannanali]

I will fix the red things

[willnorris]

Very cool, thanks for the contribution! A few initial comments:

• Google does require having CLAs from contributors, so please see that comment above about how to sign. Also, it looks like the email address used in the commit is connected to the @thisisacoder GitHub account. That's likely to cause problems with the CLA checker, so you might want to change that to an address connected to your @abdulhannanali account.
• Would you mind removing the refactoring changes from this PR? It makes things a little harder to review, and I'm not sure I'd want to make those changes anyway. I'll explain why:
  • While you're correct that the GenericTest type and helper removes some repeated code, it adds an extra layer of indirection that's not really necessary, and actually makes things less flexible, for example if we want to add a new field to the test struct for a given test, or do an extra step in the for loop. You'll find that for small amounts of code like this, the preference in Go is often to just repeat it, rather than extract it into a new type.
  • moving the tag parsing logic into a separate file now means that what was once a tight, single-file package is split out across files. For really large packages, being able to organize code across files is of course really helpful, but here there's not really much benefit.
  • defining constants for tag values does somewhat make sense. And if they were being used in multiple places, I would certainly agree with you. As it is, since they're all only ever used once (I think?), there's no bigger risk of typoing something with them appearing inline or in constants. And again, now when reading the code I have to reference what the value of the constant is, rather than the string just being right there.

As for the other further improvements you mentioned...

• I don't mind adding support for multidimensional arrays if there's an actual need for it. I intentionally don't add every possible permutation of data structure just because it's theoretically possible, since that obviously just adds a lot of bloat to the package.
• I wouldn't bother with any kinds of micro-optimizations... this isn't the kind of library where the cost of string comparisons is going to have any measurable impact. This library is typically used with clients making network calls, which is almost always going to be the slowest part.
• As for benchmarks... 🤷🏻. I have no idea how this compares with similar libraries, but I'm also not too terribly concerned. I'd actually prefer people use other packages if they suit their needs better... that's why we link to them in the README. This package was originally built for the go-github library, and that's all I've ever used it with. But by all means... the benchmarking capabilities of the testing package are actually super fun to play with, so I'd highly recommend trying them out. There might be other packages that would be better suited to looking for optimizations, though.

[abdulhannanali]

@willnorris

Thank you for patiently explaining the practices in Golang ecosystem and being willing to accept my contribution, I am happy to learn more about the practices within the Golang ecosystem, and would make sure to propose any such refactoring changes with the collaborators before right away making them.

• I have removed all the refactoring changes and have revised the PR to include only the changes related to the enhancement

Following are the refactoring changes removed:

• GenericTest logic has been removed in order to give way for more flexibility as you described
• Constant strings have been removed in favor of string literals
• Tag parsing logic has been re-included within encode.go

[willnorris]

There's still a fair number of changes in this PR that aren't actually relevant to #8, which are making it more difficult to see what is actually necessary to satisfy that feature request. For example, is the new urlTag type relevant? I don't think so?

There's also a fair amount of additional whitespace added, and test variables renamed from tt to test. It's common to use tt for test cases like this in Go. I'm not entirely sure why, and I'm sure there's a better reference, but here's a blog post that mentions it: https://www.arp242.net/go-testing-style.html. So those really shouldn't be changed here.

I'm also not entirely sure if the new embeddedStruct type is necessary. I think it might be, but it's difficult to see that at quick glance with the other changes mixed in.

See if you can distill this down to the simplest change that would satisfy the needs of #8.

[abdulhannanali]

@willnorris

Sorry for the delay in the updates, the embeddedStruct was added because for nested structs it seemed necessary to pass down the scope. I have decoupled the embeddedStruct now and created a separate map with the name scopes and type map[*reflect.Value]string which stores the scope value for the embedded value if required.

Let me know if you prefer this over the other. Thank you for sharing about the tt article, I will be using this from now on for any future tests too.

Additional white space and urlTag has been removed too

[willnorris]

Sorry, I must have missed seeing @abdulhannanali's last comment. Thanks for that cleanup, @abdulhannanali!

So, it's been a while since I've looked at this, but looking again now I think there's some inconsistency in how arrays of structs are handled. If I'm reading this right (and I haven't actually tested this with code yet), it looks like arrays of structs are only expanded as embedded structs if the indexed tag option is present. Otherwise, it's always encoded as its string value. I think that makes sense in cases where a delimiter is specified, since the expectation is that the final result is a single delimited string. But when no delimiter is specified, or when the brackets option is used, then I think it makes sense to use the same logic for expanding the embedded structs. Similarly for array of structs that themselves implement the query.Encoder type... we should use that EncodeValues implementation rather than the string value (or perhaps even rather than treating it as an embedded struct).

So while I agree that the way we handled arrays of structs today (always using their string value) is certainly not ideal, I'm thinking that a proper solution is going to be a bit more complicated and may require some refactoring. I can't spend much more time looking at this this morning, but will make myself a note to come back to this.