why +1?

The contents may not have items.

Ok, so the case in line 62 should not happen?

u.Object may or may not contain items. We can remove if in the loop because the key will be overwritten anyway. It made sense to have it before (in the previous PR) because a deep copy was made here and skipping items reduced the unnecessary work. WDYT?

I've removed the if.

this is unrelated. Can you move it to another PR? (I cannot approve this).

Most likely it was causing a failure because 123 literal has an int type by default but deep copy only likes float64 and int64. But this fix is no longer needed because we are not making a deep copy.

Can you check that the actual object stays the same? Make a deep copy before and then compare afterwards.

2018

does this really exist?

Yes, without this change (and below for completeness) the newly added check in the test (deep copy and compare) was failing. https://play.golang.org/p/KASoa2DeTnp

This will change the output - this code used to produce non-nil maps and slices for typed nils but now will produce typed nils. JSON encoding will change - typed nils as null in JSON (https://play.golang.org/p/Uz2ajLEQx9A).

I think this change is a good one - it makes the deep copy more precise.

We should never see typed nils somewhere deep inside IMO. On the toplevel maybe.

Compare https://play.golang.org/p/R45uKKqDsm6

Yes, we should not see them, but they exist. I'm fine with removing this fix from this PR if that's what you think is best. However I think that way deep copy is more correct. It just mirrors the input precisely. If there are no typed nils, we don't introduce them here. If they create a problem somewhere then the place that "spawns" them should be fixed. Or, even better, a place that does not handle them well could be made more robust.

Also, I'm not sure what exactly you wanted me to compare? typed and untyped nils are both marshaled into null, yes :)

Have we ever seen this "cannot deep copy" panic? You say that they show up. Do you know where?

Panic? Current code does not cause any panics AFAIK. Are you talking about failing test without these changes? It does not panic, it just fails because objects are not equal. They are not equal because the deep copied Object field ("expected" in the test) is an empty map and the original is `nil.

func TestNilUnstructuredContent(t *testing.T) {
	var u Unstructured
	uCopy := u.DeepCopy()
	content := u.UnstructuredContent()
	expContent := make(map[string]interface{})
	assert.EqualValues(t, expContent, content)
	assert.Equal(t, uCopy, &u)
}

--- FAIL: TestNilUnstructuredContent (0.00s)
	Error Trace:	unstructured_test.go:31
	Error:      	Not equal: 
	            	expected: &unstructured.Unstructured{Object:map[string]interface {}{}}
	            	actual: &unstructured.Unstructured{Object:map[string]interface {}(nil)}
	            	
	            	Diff:
	            	--- Expected
	            	+++ Actual
	            	@@ -1,4 +1,3 @@
	            	 (*unstructured.Unstructured)({
	            	- Object: (map[string]interface {}) {
	            	- }
	            	+ Object: (map[string]interface {}) <nil>
	            	 })

why not keep the nil and remove the if clause above?

Good question. My IntelliJ is complaining that nil is not a valid type but Go compiler is happy 🤷‍♂️ It actually works fine https://play.golang.org/p/PBIyhFgWn8H
I'll revert this change.

Don't feel strong about it. Was just curious.