-
-
Notifications
You must be signed in to change notification settings - Fork 39
Add initial segments and cm methods for PR review #220
base: master
Are you sure you want to change the base?
Conversation
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. |
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). |
@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):
with the difference being that
|
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) |
@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:
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 But, if you need to include a parser function, do it. I know the JSON Adobe returns is horrible! |
Got it. Yes, makes sense. Let me see what I can do, particularly once I simplify. |
@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 |
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 ofSegments_Get
orCalculatedMetrics_Get
, but is referenced and provides additional functionality, mainly to parse specific parts of certain return structures (shares
anddefinition
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