Move FamilyName() to an "enumerated" type.

[hhhjort]

Created new package "family" to avoid a circular import (pbs needs to know about family, adapters imports pbs).
The family.Name type extends into pbs.PBSRequest.GetUserID() and cookieSync()

This should provide some safety around handling FamilyNames in adapters.

[dbemiller]

I would lean towards naming this package constants, to make it more reusable. Named family, nothing else will go in there.

There are a few other "magic string constants" which are shared throughout several other PBS modules though. For example, BidderCode and ResponseStatus. It'd be nice to have a place to move them all to, as time permits.

[dbemiller]

What made you decide to switch back to strings here?

[hhhjort]

It was either dig in here to make sure I caught every usage of UserIDs, or put it off for a later PR. Seems a bit more manageable in chunks rather than starting off with a huge PR as my first.

The comment above about "family" makes sense and I will work on it, and I will take a look at how much UserIDs is actually referenced and consider pushing familyName all the way down there.

[dbemiller]

gotcha... ok.

The main benefit of having a type at all is to make bugs less likely. Right now, req.UserIDs[adapter.Name()] is a really easy mistake to make. It would work most of the time... but not always--which makes it vulnerable to subtle, hard-to-find bugs.

Working in smaller chunks is also a noble goal, though, and I personally tend to make much larger diffs than average (to the point that it annoys people sometimes).

I'm not sure what the answer on "how to merge well" is. I do definitely think familyName should be pushed around the whole codebase, though. The main value is to help document which parts of the code are implicitly "the same as" other parts, and make bugs due to miscommunication less likely.

[hhhjort]

Been looking at pushing FamilyName all the way down. The difficulty comes in at PBSCookie, which eventually gets marshalled into json format and that is converted into a string and set as a cookie. Since this hits an external interface (how we store ID cookies in a user's browser) this would be a breaking change if we changed how it is done. Ideally the solution should be using the values from constants to convert the internal int indexed array with the string indexed map that goes into the cookie. The current build has family moved to constants, but no work done yet on pushing FamilyName down further.

[hhhjort]

I have a branch now that uses FamilyName in PBSRequest, but leaves PBSCookie alone. Utility functions to convert between the array and map. Down side is there are now 2 ways to index UserIDs in an array/map, which could lead to confusion down the line. But since the map indexes on a string, and the array on int, compiler will catch any confusions. The only way to push this further is to create a second PBSCookie struct, and convert between array based PBSCookie and map based PBSCookie when reading/writing the actual cookie. Not sure when we hit diminishing returns on this.

Do you want this branch merged into this pull request, or open a second PR?

[dbemiller]

Either way should work, as long as you have separate commits. You can always git reset HEAD~numCommits and git push -f branch if you want to delete those changes later.

Or you could just open a PR between the two branches in your own repo, and I can take a look there.

Your call on which one you'll find easier to work with.

I do definitely find the phrase "2 ways to index UserIDs" a bit disturbing... but I'm not sure exactly what you mean by it.

Duplicating PBSCookie structs would be a clearly bad idea though. What do you mean by "the only way to push this farther"? What issue prevents you from pushing it into the PBSCookie which already exists?

[dbemiller]

Also FYI: there are some other changes to the PBSCookie in the pipeline. I'm not sure which one of us will merge first... but that might impact the issues you're seeing a bit ^^.

[hhhjort]

Set up a PR on my fork.

"2 ways to index UserIDs": We have a slice of UserIDs that are indexed according to the int value of FamilyName and we have a map of UserIDs that are indexed by the string values. This could be a little confusing when writing code, but the compiler will ensure we don't have the two index methods confused in the end.

The issue with PBSCookie is pbs.ParseUIDCookie() and pbs.SetUIDCookie() which use json.Mashal and json.Unmashal to convert between the PBSCookie struct and the JSON representation that gets stored as the actual cookie in the browser. To convert between the struct and the JSON, we need a struct that matches the structure of the JSON. The JSON currently uses a dictionary/map to index the various UIDs, so we need a PBSCookie struct that uses map in order to match. If we want a PBSCookie struct with an array for internal use, we would have to convert between PBSCookieInternal and PBSCookieJSON in these two functions, or implement our own JSON parser to work around the mismatch between the array of UIDs and the dictionary of UIDs. Converting between the two structs would not be difficult since I already have functions to convert between the map and array, but I haven't verified that there isn't any other code that bypasses this functions to access the cookie directly.

So it might not be too bad to rename PBSCookie to pBSCookieJSON, and define a new PBSCookie with an array of UIDs. Lower case 'p' as pBSCookieJSON should only be used in the two cookie functions, and thus not needed outside the package.

[hhhjort]

Need to resubmit from a branch rather than master.