fix: disambiguate variable defaults with "empty" values from undefined

[jstewmon]

closes #23

[jstewmon]

I had to add a flag to indicate if the default was defined to be able to handle the case where default = null. Originally, I tried using reflection to check if the default had a type, but that didn't work when the default was null because the Default will be nil when default is null.

[jstewmon]

The tests are just failing a b/c of the new attribute I added to the interface. I'll be happy to fix those if this is an acceptable fix for the issue. :-)

[jstewmon]

I'm not familiar with custom JSON serialization in go, but from the digging I've done this afternoon, it seems like the most straightforward solution is to remove the omitempty annotation from Default and include HasDefault.

I suppose another alternative would be to make Default a complex type that has a flag and a value, though I'm not sure that would work in practice.

Would love to get some guidance on a sound approach. 🙏

[jstewmon]

I figured out that just wrapping the Default in a struct was sufficient to disambiguate whether or not a default was defined and is trivially unwrapped with a custom MarshalJSON. 😄

[jstewmon]

@mildwonkey would you be so kind as to take a look at this and provide feedback? We're getting a lot of leverage out of this project, especially maintaining module docs, and would love to have the variable requirements accurately documented. 🙏

[moritzheiber]

@jstewmon Great work, it's been annoying to see this happening @radeksimko Would love to get this merged if you have some time :heart:

[willychenchen]

Could we get this reviewed and merged in, please? Thank you.

[mildwonkey]

Hi @jstewmon ! Thank you for submitting this PR.

I understand and agree with the desire to differentiate between unset and empty default values. However I'd like to suggest a different approach, which is more in line with how Terraform itself models that idea: add a boolean for whether the variable is required (i.e. true if there is no default value; false when there is a default, even if the default is "empty"). Does that sound reasonable to you?

[jstewmon]

@mildwonkey , sure - I'll add a new commit, so we can review how that looks.

I didn't do that originally b/c I thought it would be less intuitive to remove omitempty from Default, so that you'd always have Default and have to check another field to determine if Default is significant.

But, sounds like there is more upside to doing it that way to align more closely with Terraform.

[jstewmon]

@mildwonkey new commit to add required field is ready for you to review!

[jstewmon]

Hi @mildwonkey 👋 would love to get your feedback on the new approach when you have a few minutes. 🙏

[khos2ow]

Is there any ETA on this? We have a feature request at segmentio/terraform-docs#176 which depends on this and I'd rather use the functionality right from terraform-config-insoect than try to re-implement it within terraform-docs. Also I can help on anything on this PR if nees be.

[mildwonkey]

Hello @jstewmon and sorry again for the long wait! Thank you for making the latest update. I'd like to get this merged. Do you want to update this PR and resolve the conflicts? If you aren't interested in working on this PR anymore (it has been a long time, I know!) I can resolve the merge conflicts. Cheers!

[jstewmon]

Hi @mildwonkey , I've rebased against master. Would you like for me to squash out the original commit?

[mildwonkey]

@jstewmon only if you want to re-write your commit history yourself - I usually use squash and merge, so it isn't necessary.