Add initial segments and cm methods for PR review

[slin30]

trafficstars

Hi Randy,

At long last, am submitting an initial PR for review. Also see the updated README in the private gitlab mirror for additional notes/context.

In said README, a designation of [OPT] indicates the function is optional, i.e. it is not technically required for either of Segments_Get or CalculatedMetrics_Get, but is referenced and provides additional functionality, mainly to parse specific parts of certain return structures (shares and definition specifically).

Also note I put together a quick script to aid in copying the target files into the required target directory structures to make this process easier/more robust:

https://gitlab.com/slin30/SCAdmin/blob/master/Integration/pull_files.R

Look forward to comments,

Wen

[randyzwitch]

Thanks for submitting! There's a lot going on here, so hopefully I can review this in the next week or so.

In general, I'm not a fan of having text files for the tests. I get why you have them, but ultimately, my only concern is that live functions parse correctly. With text files, we have to hope that Adobe hasn't changed their structure.

[slin30]

Definitely agree re: testing with text vs. live. I don't have a personal account that would let me reliably test live, but since (it seems) you do, I am more than happy to convert to a live test workflow. Let me know if there's anything I can supply to make this easier (e.g. a few test segment configurations).

[randyzwitch]

@slin30 Thanks again for submitting this, I know it takes a lot of effort to contribute to libraries that aren't yours!

Before, I had mentioned that I didn't really have coding standards for this library, but this PR has helped me outline some of my thoughts. Please take these comments in the spirit of making the package better, and not as a commentary on your coding style (which to be honest, far exceeds most of this package):

• Public method names should be CamelCase, no dots or spaces. Please rename the functions as CalculatedMetricsGet and SegmentsGet. Method arguments should be separated with periods, like access.level.
• Functions should have the minimum number of options to lessen cognitive load for the user, as the majority of the users are likely not strong R programmers. Removing options like sort, fields, filters and collapse_simple reduces a users' options to "What do I have access to?" (access.level) and "What is the definition of this id?" (selected)
• Every method returning data should be parsed; like the previous comment, assuming users can parse complex data types in a data frame or would know whether they want data types parsed is unnecessarily complicated. In general, returning data in "tidy" form is desired, but it can depend on what is returned by Adobe
• The ideal function call in my mind is what is returned by:

                                           "favorite", "modified", 
                                           "owner", "reportSuiteID", 
                                           "shares", "tags")
)

with the difference being that fields shouldn't be an argument, but rather, have all columns returned. Thus, what is returned by the example above should be the result of needs_parsing_3 <- Segments_Get() or needs_parsing_3 <- Segments_Get(access.level = "all")

• Providing tests is not required; due to way Travis/AppVeyor works, these tests will always fail for users other than me as the CI tools don't authenticate pull requests. Examples in the documentation section are sufficient, they will be copied over by me as test examples.
• Internal-only functions are okay, but parsing should try to be done using plyr, stringr or base R methods as possible. This is to remove need to maintain the fewest number of functions not relating to calling Adobe API

[randyzwitch]

So the question now becomes @slin30, would you be interested in taking a shot at these revisions? I realize it's a lot, and diverges from your internal package quite a bit. The good part is that your code does work for my account, so most of the recommendations are around making your PR simpler in its scope.

(I'll also add a section for contributing to the library, so that others will have guidance)

[slin30]

@randyzwitch, thanks for the comments/feedback! I am happy to take this on, and should be able to re-submit within a couple weeks.

The only comment I am unclear about is:

Internal-only functions are okay, but parsing should try to be done using plyr, stringr or base R methods as possible. This is to remove need to maintain the fewest number of functions not relating to calling Adobe API

I didn't think I used any libraries outside the scope of what your package currently imports, for the code targeted for contribution, although I'll double-check. Or am I misunderstanding?

[randyzwitch]

I didn't think I used any libraries outside the scope of what your package currently imports, for the code targeted for contribution, although I'll double-check. Or am I misunderstanding?

You're right, you aren't importing any new packages. What I meant was, rather than defining a function for collapse_simple_listcol.R, check first to see if that already exists in a package being imported. That way, a package like plyr can manage the formulas for data processing, and the majority of the functions defined in RSiteCatalyst will just be logic specifically for the Adobe API

But, if you need to include a parser function, do it. I know the JSON Adobe returns is horrible!

[slin30]

Got it. Yes, makes sense. Let me see what I can do, particularly once I simplify.

[slin30]

@randyzwitch, have not forgotten about this; been swamped with work, so apologies for the silence. My main challenge/question is how you suggest we handle the definition return, due to the likelihood that we won't be able to parse the definition in many cases. The API will not return a definition for segments that have a time component, such as a THEN operator. Nested segments of varying flavor also pose some interesting challenges.