Improve default value handling for primitive types

[neomantra]

As discussed #285, there are issues with the marshaling / unmarshalling of the defaults of basic types, when the default is not nil. Capturing it here as an issue.

There are a few ways of handling it, such as using pointers or custom types.

[neomantra]

This was a helpful blog entry, which break down the issues and presents some solutions. Optional JSON Fields in Go.

1. Use pointers to the primitive types (*bool) with omitempty annotation, users write config.Show = opts.Bool(true)

   • 👍 does the trick, omitted so default is delegated to JSON reader
   • 😢 Breaking change for user code:
   • but we also can introduce slowly (like the new Show and then warn users that it is coming)

2. make a custom type with a custom marshaller

   • does the trick, more complicated and intrusive, especially since the only "custom" aspect is the defaulting
   • same user introduction issues

3. pre-fill a struct with all the defaults exactly matching echarts

   • this will work, but is harder to maintain as go-echarts maintainers need to track upstream, rather than simply delegating like the other options
   • zero user impact, except for where defaults have been wrong (perhaps Snap?)

---

I vote for 1 using pointers as the design choice, with a policy that it is only introduced for fields with known non-nil defaults. [Otherwise, it might become cumbersome for maintainer and user.]

[neomantra]

I am pushing a simple implementation of (1) above with my PR #285 ... I was wondering where to document this, and I won't make documentation part of that PR.. Is it OK to put in a section in the README? Or just in the GoDoc?

[neomantra]

I just tried this out and here's what my dependent code saw:

internal/graph/graph.go:134:11: cannot use true (untyped bool constant) as *bool value in struct literal

It was very easy to go to that line and update it

	AxisPointer: &opts.AxisPointer{
-		Show: true
+		Show: opts.Bool(true),
		Snap: true,
	},

[Koooooo-7]

Hi @neomantra , thanks a lot for your input on it !

I vote for 1 using pointers as the design choice, with a policy that it is only introduced for fields with known non-nil defaults. [Otherwise, it might become cumbersome for maintainer and user.]

I agree that the *bool stuff is the most reasonable change but it brings breaking stuff. If we bring it in the v2 sounds weird tho. Maybe it should be raise in v3 or develop in double release both v2 v3 ... later and further?

And about those fields with known non-nil, I think it may include those primitive types [bool, int, float ] we used in here.
If we just wrapper it to Bool, Int and Float for *bool, *int, and *float as wrapper classes and left the string and interface{} , I think it may lose the consistent personally. instead, we may need some String things (or not...).

Besides, If we gonna make it in a brand new v3 directly, new fresh go-echarts could come again ( there is the POC works on this branch ). When there has a clear pictures about it , I will let you know asap, and then, we could move onto it . If you also have interests on the v3 that would be great.

Go big or go home.

[neomantra]

I saw the v3 branch recently and like the direction so far. I agree this should definitely be done in that version. Whether to do also do it on v2 is a matter of timeliness of v3 and the expected longevity of the v2 branch.

The great thing about the change is that it is not silent and it is easy to fix. Are there massive go-echarts codebases that would be deeply hurt by this change?

Here's a ported example from my codebase, to help think about it:

// original
volumeBarChart := charts.NewBar()
volumeBarChart.AddSeries("volume", nil,
	charts.WithItemStyleOpts(opts.ItemStyle{Color: "#7fbe9e"}),
	charts.WithBarChartOpts(opts.BarChart{Type: "bar", XAxisIndex: 1, YAxisIndex: 1}),
	charts.WithEncodeOpts(opts.Encode{X: "open_ms", Y: "volume"}))

// all opts wrapped
volumeBarChart := charts.NewBar()
volumeBarChart.AddSeries("volume", nil,
	charts.WithItemStyleOpts(opts.ItemStyle{Color: opts.String("#7fbe9e")}),
	charts.WithBarChartOpts(opts.BarChart{Type: opts.String("bar"), XAxisIndex: opts.Int(1), YAxisIndex: opts.Int(1)}),
	charts.WithEncodeOpts(opts.Encode{X: opts.String("open_ms"), Y: opts.String("volume"})))

I think that we do need String for the case where there is a string whose default that is not "".

Like you say, I think the approach of documenting this feature generally and then explicitly using it for fields that need it. If it's a string field that needs it, that's OK.

[Koooooo-7]

I saw the v3 branch recently and like the direction so far. I agree this should definitely be done in that version. Whether to do also do it on v2 is a matter of timeliness of v3 and the expected longevity of the v2 branch.

Yep, the most important thing in front of me is how we should handle the v2 and v3 branches. since I could not say there has an upgrade from v2 -> v3, so we need drop v2 soon. It is more like a Lite and a Full modes.

• v2 is more straightforward and out of box, but it is hard to make complex charts on current codebase/apis.
• v3 would be more flexible and customize but I m not quite sure whether we could make it more handy. Maybe it contains a balance in further.

The great thing about the change is that it is not silent and it is easy to fix. Are there massive go-echarts codebases that would be deeply hurt by this change?

To handle the default value stuff in v2 is not so hard and we could solve it in v2 in the way you mentioned later.
Ok, let's raise a PR in v2 to fix it now and take steps to v3 as well.

I think that we do need String for the case where there is a string whose default that is not "".

Like you say, I think the approach of documenting this feature generally and then explicitly using it for fields that need it. If it's a string field that needs it, that's OK.

Agree. TBH, I know that we are lack of docs for now and then. 😅 Actually, it was a plan to get a doc site to clarify options and examples but it is always delay.

[neomantra]

I haven't looked recently, but I did appreciate the new code organization of v3. But I didn't really look at how the interface changed -- I didn't see an issue capturing what you are trying to do there.

Ok, let's raise a PR in v2 to fix it now and take steps to v3 as well.

I can take a stab at this, but also didn't want to duplicate work with you or anybody. I would do what we described... go through the options, find ones that have non-zero defaults, and add the wrapper type to those.

[Koooooo-7]

Hi @neomantra , sorry that I'm more into the v3 stuff recently.
About the updates on v2, I would appreciate it that you could do the changes about what we described.
And about the v3, I need some advices as well....

[neomantra]

OK, feel free to assign this to me (I don't have permissions to do so). Not sure on timing, but I'll stab at it. But if you are pushing on v3 would it be better for me to advance that with you? I guess it depends on your timeline.

FYI, another feature I'm background-working on is Custom charts. I have it working, but not in a way I love. I should just throw out a WIP PR. Or is that being addressed with your v3 work?

[Koooooo-7]

OK, feel free to assign this to me (I don't have permissions to do so). Not sure on timing, but I'll stab at it. But if you are pushing on v3 would it be better for me to advance that with you? I guess it depends on your timeline.

Assign to you now, take your time is fine. 👌
It would be great that if you could check on the v3 and give me some concerns or just contribute on it together. 😄

FYI, another feature I'm background-working on is Custom charts. I have it working, but not in a way I love. I should just throw out a WIP PR. Or is that being addressed with your v3 work?

I think it may have a WIP pr drop here, although it based on v2 . we could take a look on it and think about how to integrate it into v3 as well. About the v3, it just starts up and I m quite dizzy about how to achieve those apis. 😅