WIP GH-56 Refactor to use pointers for all fields. #83

ojongerius · 2017-01-27T02:00:46Z

Refactors fields to be pointers
Adds a few setters for common types.

Needs work to let acceptance tests TestCreateAndDeleteMonitor and TestCreateAndDeleteDowntime pass.

Needs discussion of getters next to the current setters.

… return pointers for common types.

ojongerius · 2017-01-27T02:05:31Z

@zorkian feel free to push changes to this branch, I've pushed it here with that in mind.

ojongerius · 2017-01-28T09:03:05Z

Made all acceptance tests pass too.

zorkian

Okay so this is looking good, thank you for doing this work! Some general thoughts:

Should we switch from int to json.Number everywhere? In most cases it doesn't matter but it feels like it might be useful to standardize instead of having to guess/worry about getting it wrong.
For helpers, what do you think about the way protobufs create GetFoo helpers for each member and handles returning the language-default rather than a possibly-nil which is right most of the time, and if someone really wants to see if it's nil they can inspect the member. (The comma ok idiom is very verbose.) I'm interested in keeping the API simple to use, but correct. Any thoughts?
I mentioned in one place but there is inconsistency around the deployment of omitempty or not. Maybe we should just put it everywhere and leave it up to the user to make sure they set the fields they need to set. Since the DD API will error if they do it wrong, it saves us from getting mismatched expectations -- i.e. if DD decides to make something optional and the library is forcing it, someone has to patch it before they can use it which is a bummer.

But yeah, overall this is a great start. Would love to hear your thoughts on the above.

And as an aside, I'm going to do an experiment -- protobufs actually have a marshal-to-JSON option. I'm wondering if I write a proto definition for these API methods if it would Just Work or if the JSON it outputs can't be made compatible with DD's API? Worth a shot anyway.

zorkian · 2017-01-30T17:26:31Z

dashboards.go


-		// For hostnamp type graphs


zorkian · 2017-01-30T17:30:32Z

alerts.go

-	Silenced     bool   `json:"silenced,omitempty"`
-	NotifyNoData bool   `json:"notify_no_data,omitempty"`
-	State        string `json:"state,omitempty"`
+	Id           *int    `json:"id,omitempty"`


This is unrelated to your changes here but I'm noticing that it looks like inconsistent usage of omitempty. The Alert struct uses it on every member but Comment only uses it on optional members.

True, I'll add omitempty to all fields 🤞

zorkian · 2017-01-30T17:31:37Z

helper_test.go

+ *
+ * Please see the included LICENSE file for licensing information.
+ *
+ * Copyright 2016 by authors and contributors.


zorkian · 2017-01-30T17:32:11Z

helper_test.go

+	"github.com/zorkian/go-datadog-api"
+)
+
+func TestBool(t *testing.T) {


Did you want this to do anything?

Nope! Thanks for spotting it.

zorkian · 2017-01-30T17:33:16Z

helpers.go

+ *
+ * Please see the included LICENSE file for licensing information.
+ *
+ * Copyright 2016 by authors and contributors.


2017 already indeed..

zorkian · 2017-01-30T17:38:31Z

helpers.go

+
+// Bool is a helper routine that allocates a new bool value
+// to store v and returns a pointer to it.
+func Bool(v bool) *bool { return &v }


Note to self: out of curiosity, does the compiler inline this and go straight to heap? Or does this get passed in on the stack, then heap allocate, then copy, then return pointer? /curiousminds

The -m flag to the rescue; it looks like the first: inline and straight to the heap:

go build -gcflags=-m helpers.go # command-line-arguments <SNIP> ./helpers.go:15: can inline Bool ./helpers.go:15: &v escapes to heap ./helpers.go:15: moved to heap: v <SNIP>

Nice! Well, that makes me happy. Thanks for checking. :)

zorkian · 2017-01-30T17:39:59Z

helpers.go

+		return *v, true
+	}
+
+	return b, ok


I know that semantically this returns the zero-value for these which is equivalent to return false, false but it seems a bit non-standard to do it this way. What do you think about explicit return false, false here?

Or, I could possibly see return b, false but at least the ok should be defined?

Agreed, and happy to make that change.

zorkian · 2017-01-30T17:41:54Z

integration/dashboards_test.go

 	graphs := []datadog.Graph{}
 	graphs = append(graphs, graph)
 	return graphs
 }

 func assertDashboardEquals(t *testing.T, actual, expected *datadog.Dashboard) {
-	if actual.Title != expected.Title {
-		t.Errorf("Dashboard title does not match: %s != %s", actual.Title, expected.Title)
+	if *actual.Title != *expected.Title {


Do we want to be using helpers here? (And elsewhere where we just dereference pointers?) See later discussion on helpers.

My initial take on the PR was to see if we could agreement on pointers and setters, and I added the getters afterwards.

At the same rate with tests we know what can expect so it's safe to do this.

Makes sense. Well, I'm agreed on using the methods like Int and String etc. Although if we do go with the json.Number route we should make it Number instead? (Oh ugh, nevermind, I forget Go can't overload arguments... oh wait, I guess we could make it take an interface{} and unbox it ourselves??)

zorkian · 2017-01-30T17:42:28Z

integration/downtime_test.go


-	// Set ID of our original struct to zero we we can easily compare the results
+	// Set ID of our original struct to zero so we can easily compare the results


ITT comments are hard. Thanks for fixing!

* Bring years up to date in helper files * Remove empty test.

ojongerius · 2017-01-31T04:12:16Z

Should we switch from int to json.Number everywhere? In most cases it doesn't matter but it feels like it might be useful to standardize instead of having to guess/worry about getting it wrong.

I agree we should be consistent. I'll keep off doing that until we are sure what direction we are going.

For helpers, what do you think about the way protobufs create GetFoo helpers for each member and handles returning the language-default rather than a possibly-nil which is right most of the time, and if someone really wants to see if it's nil they can inspect the member. (The comma ok idiom is very verbose.) I'm interested in keeping the API simple to use, but correct. Any thoughts?

I don't have experience with protobufs, can you share a code snippet on how a client would use it, and how we would code those getters? Definitely a fan of finding a clean way of not having to use the comma ok idiom too.

I mentioned in one place but there is inconsistency around the deployment of omitempty or not. Maybe we should just put it everywhere and leave it up to the user to make sure they set the fields they need to set. Since the DD API will error if they do it wrong, it saves us from getting mismatched expectations -- i.e. if DD decides to make something optional and the library is forcing it, someone has to patch it before they can use it which is a bummer.

Agreed and implemented.

And as an aside, I'm going to do an experiment -- protobufs actually have a marshal-to-JSON option. I'm wondering if I write a proto definition for these API methods if it would Just Work or if the JSON it outputs can't be made compatible with DD's API? Worth a shot anyway.

Interested to see how you go! Will Norris started to port go-github to use protobufs but ended up reverting that and cherry picking some of protobufs functionality. See this comment google/go-github#19 (comment) .

This field defaults to 300 if empty, but we also need to support setting it to 0 so we use a *int pointer.

Pull in some of ojongerius's changes from https://github.com/zorkian/go-datadog-api/pull/83/files#diff-01127be9ed6b6352616a2f1c10bc3540R29

…herry-picked).

zorkian · 2017-02-01T06:59:15Z

I don't have experience with protobufs, can you share a code snippet on how a client would use it, and how we would code those getters? Definitely a fan of finding a clean way of not having to use the comma ok idiom too.

I was thinking like this: https://play.golang.org/p/sh2M9cIs-2

That'd be pretty annoying to have to build ourselves though, but maybe some awk/sed/scripting can generate it appropriately. Or, actually, something like this:

https://play.golang.org/p/Cq9dWFqAd0

If we throw that in some autogen/ package it can import all the structs and then dump out the appropriate methods. If we were really fancy it could just automatically rewrite the source files or something. (Or possibly some magic with gofmt -r but I don't know how advanced that can get.)

Okay sorry, I've gone off a bit into the weeds here. I think easy-to-use is a noble goal if we can achieve it, but I'm not asserting that having Get... and Has... methods is the way to go. I kind of like the simplicity but it is a lot of work to maintain.

ojongerius · 2017-02-02T00:51:19Z

Am reaching out to Datadog folks if we could get a service descriptor from them.

Generating the whole library a la https://github.com/google/google-api-go-client or https://github.com/aws/aws-sdk-go would be an direction I'd be interested in to work on.

@zorkian thoughts?

ojongerius · 2017-02-16T01:28:46Z

Datadog is not going to work on publishing their API any time soon 😞

On a more positive note I've coded something up that generates HasXx, GetXx and SetXx. Using it would look like:

	m := datadog.Monitor{}
	c := datadog.Creator{}
	
        // Use SetXx:
        c.SetName("Joe Creator")
	m.SetName("Monitor all the things")
	m.SetMessage("Fat electrons in the lines")

	// Use HasXx:
	if m.HasMessage() {
		fmt.Printf("Found message %s\n", m.GetMessage())
	}

	// Or maybe you prefer a tuple with the value and a boolean if it was ever set
        // and use GetXxOk
	if v, ok := m.GetMessageOk() ; ok {
		fmt.Printf("Found message %s\n", v)
	}

I still like having the other helpers around, and keep support for doing:

	// You could still initialize the struct in one swoop
	_ = datadog.Monitor{
		Name: datadog.String("Monitor other things"),
		Creator: &datadog.Creator{
			Name: datadog.String("Joe Creator"),
		},
	}

I'll push up some changes to this PR hopefully today, else tomorrow.

* Add code to generate accessors. * Update Makefile to have generate target * Refactor generic GetXx functions that returned a tuple to GetXxOk * Add comment to let 'go generate' find code generation.. code. * Update integration test to use some of the generated accessors.

ojongerius · 2017-02-16T06:49:39Z

@zorkian thoughts? This PR is getting a unwieldy, and has been open for a little while.

Note that gen-accessors.go is very heavily inspired by this open PR: google/go-github#543. It basically just adds Setters, GetOk to the Getter already there, and some comments to made it a little more readable for newbies to code generation like myself.

I think this gets us in a situation where we can have both pointers and keep the API relatively nice to use.

In the (near) future I am inclined to go one step further and generate not from handcrafted Gocode, but an OpenAPI specification.
Although Datadog does not want to rush providing us with one we could be more future proof by converting the structs to YAML or JSON, and generate the structs, the helpers, and even most or all of this library from it. By the time Datadog starts publishing their API it's just a matter of stop maintaining our descriptor and consume theirs.

… and add build tag.

…h versions.

…s on how to regenerate code.

ojongerius · 2017-02-17T06:52:03Z

cmd/tools/gen-accessors.go

+
+// processAST walks source code files, looks for struct definitions, and calls appropriate helper functions
+// to add generate accessors for type fields are star expressions -ie: pointers
+func (t *templateData) processAST(f *ast.File) error {


This currently only goes one level deep. If we want to support nested structs this will needs refactoring. Although we have nested structs at the moment, I'm not super keen on them.

My preference at the moment is to get all this merged into v2, and if we agree on the direction, support accessors for nested structs in a separate PR. It's not much work but this PR is a big of a change as it is.

AFAIC, before we make v2 the default branch we can safely to this and test with v2.

@zorkian thoughts?

This is a much fancier approach than the one I had for sure! I think ATM having top-level only is fine, and we can see how things develop.

zorkian

LGTM. It looks like you didn't run the generator yet/that code is not submitted to the PR? (That's fine, just noting and making sure I understand.)

And to make sure I remember, this means now we'll have to be thoughtful about making sure people go generate if the structs change, and/or we'll have to do that. (Ideally the former.)

I do like the shape this has taken, though! Should be a good way to give people correctness but still a pretty flexible/easy-to-use library.

Let's get this landed on a v2 branch and, when we're satisfied it's stable/has everything, move the pointer for master?

zorkian · 2017-02-17T23:49:45Z

cmd/tools/gen-accessors.go

+
+// processAST walks source code files, looks for struct definitions, and calls appropriate helper functions
+// to add generate accessors for type fields are star expressions -ie: pointers
+func (t *templateData) processAST(f *ast.File) error {


This is a much fancier approach than the one I had for sure! I think ATM having top-level only is fine, and we can see how things develop.

ojongerius · 2017-02-18T02:52:11Z

I did run the generator, not a 100% if it contains all the changes. All generated accessors are in datadog-accessors.go which did not show up as a diff as it is a new file. We could also chose to have the accessors closer to the code structs. Up to us.

Thanks for the review @zorkian! Yes code regeneration will have to be run manually, I imagine we could be smart about this and have a make target that verifies that code has to be regenerated. For now I will merge this and we can get fancier later.

I might have a stab at updating Terraform to use the new version of the library and see if I run into issues. We could also entertain adding instructions on how to use the new branch to master.

GH-56: Refactor to use pointers for all fields. Adds a few helpers to…

0675f9d

… return pointers for common types.

ojongerius mentioned this pull request Jan 27, 2017

Consider updating library to use pointers for optional fields #56

Closed

ojongerius added 3 commits January 28, 2017 17:20

GH-56: Set default values for test structs.

10ab901

GH-56: Fix widget test. Rename tests to make testing groups more easy.

2a67c65

GH-56: Fixup of user test.

1a5085b

ojongerius added 3 commits January 29, 2017 19:25

GH-56: Add a few getters and tests.

f18c0a7

GH-56: Update comments.

9e9f19d

Simplify Widget tests.

8a1211a

zorkian self-requested a review January 30, 2017 17:25

zorkian requested changes Jan 30, 2017

View reviewed changes

ojongerius added 3 commits January 31, 2017 12:58

Small changes implementing PR feedback.

b80c777

* Bring years up to date in helper files * Remove empty test.

Be explicit in what is returned instead of using the default value.

7126deb

Make use of omitempty consistent accross all types.

44f55f5

Jesse Szwedko and others added 3 commits January 31, 2017 18:05

Add support for new_host_delay in monitor definitions

0984f79

This field defaults to 300 if empty, but we also need to support setting it to 0 so we use a *int pointer.

Be explicit in type conversion helper definitions

2f75850

Pull in some of ojongerius's changes from https://github.com/zorkian/go-datadog-api/pull/83/files#diff-01127be9ed6b6352616a2f1c10bc3540R29

Getters and setters already declared in helpers.go (this change was c…

e1345fa

…herry-picked).

Properly encode/decode Aggregator param.

809cc0c

ojongerius added 4 commits February 16, 2017 17:40

Add generated code.

0f5ff63

Gofmt.

826e56f

fmt.

52b66b3

ojongerius added 2 commits February 16, 2017 22:47

Move generate comments to generate.go, renamed generation source file…

cccf633

… and add build tag.

Add full stop and end of Godoc string.

59ba811

ojongerius added 2 commits February 17, 2017 10:46

Regenerate code.

dcb6df3

Update README to reflect API changes and inform users on how to switc…

edd7699

…h versions.

ojongerius force-pushed the ojongerius/use_pointers_for_all_fields branch from e1758bc to edd7699 Compare February 17, 2017 03:03

ojongerius added 4 commits February 17, 2017 14:17

Update README with correction on how to use gopkg.in, and instruction…

cfa8d69

…s on how to regenerate code.

Update example to use Int helper.

53aa1ca

Improve Readme.

495a0f0

Wording on code generation.

ecbc43f

ojongerius commented Feb 17, 2017

View reviewed changes

zorkian approved these changes Feb 17, 2017

View reviewed changes

ojongerius merged commit ab12404 into v2 Feb 18, 2017

ojongerius deleted the ojongerius/use_pointers_for_all_fields branch February 18, 2017 02:52

ojongerius mentioned this pull request Feb 20, 2017

Remove nesting from Graph struct #98

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP GH-56 Refactor to use pointers for all fields. #83

WIP GH-56 Refactor to use pointers for all fields. #83

ojongerius commented Jan 27, 2017 •

edited

Loading

ojongerius commented Jan 27, 2017

ojongerius commented Jan 28, 2017

zorkian left a comment

zorkian Jan 30, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Feb 1, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

zorkian Feb 1, 2017

zorkian Jan 30, 2017

ojongerius Jan 31, 2017

ojongerius commented Jan 31, 2017

zorkian commented Feb 1, 2017

ojongerius commented Feb 2, 2017

ojongerius commented Feb 16, 2017

ojongerius commented Feb 16, 2017 •

edited

Loading

ojongerius Feb 17, 2017

zorkian Feb 17, 2017

zorkian left a comment

zorkian Feb 17, 2017

ojongerius commented Feb 18, 2017


		// Set ID of our original struct to zero we we can easily compare the results
		// Set ID of our original struct to zero so we can easily compare the results

WIP GH-56 Refactor to use pointers for all fields. #83

WIP GH-56 Refactor to use pointers for all fields. #83

Conversation

ojongerius commented Jan 27, 2017 • edited Loading

ojongerius commented Jan 27, 2017

ojongerius commented Jan 28, 2017

zorkian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojongerius commented Jan 31, 2017

zorkian commented Feb 1, 2017

ojongerius commented Feb 2, 2017

ojongerius commented Feb 16, 2017

ojongerius commented Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zorkian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojongerius commented Feb 18, 2017

ojongerius commented Jan 27, 2017 •

edited

Loading

ojongerius commented Feb 16, 2017 •

edited

Loading