Add an inspection that reports properties being set to their default value #169

matthew16550 · 2018-10-08T01:01:49Z

No description provided.

…value

matthew16550 · 2018-10-08T01:06:58Z

These providers had errors when I ran the schema extractor but I ignored them as out of scope:

terraform-provider-azure-classic
terraform-provider-google-beta
terraform-provider-oci
terraform-provider-scaffolding

And there were 4 new providers which I also ignored:

bigip
netlify
rightscale
tencentcloud

matthew16550 · 2018-10-08T01:08:01Z

schemas-extractor/build-all.sh

-  sed -i "s/__NAME__/${p:19}/g" generate-schema/generate-schema.go
-  sed -i "s/__REVISION__/$revision/g" generate-schema/generate-schema.go
-  sed -i "s~__OUT__~$out~g" generate-schema/generate-schema.go
+  sed -i '' "s/__FULL_NAME__/$p/g" generate-schema/generate-schema.go


sed on my Mac needs this

matthew16550 · 2018-10-08T01:09:08Z

schemas-extractor/template/generate-schema.go

@@ -127,8 +126,11 @@ func export(v *schema.Schema) SchemaDefinition {
 	}

 	// TODO: Find better solution
-	if defValue, err := v.DefaultValue(); err == nil && defValue != nil && !reflect.DeepEqual(defValue, v.Default) {


I couldn't see what DeepEqual was achieving here, perhaps something important that I misunderstood?

Can't remember why I've added it here. Does removing changes anything?

Using !reflect.DeepEqual(... there are 265 defaults generated. Removing it makes 3134 defaults generated. The diff is too big to compare everything but I think it is correctly finding all default values when removed.

VladRassokhin · 2018-10-08T15:39:49Z

src/kotlin/org/intellij/plugins/hcl/terraform/config/inspection/TFDefaultPropertyInspection.kt

+            }
+        }
+
+        private fun compare(value: HCLValue, defaultValue: Any): Boolean {


It's better to name it isEqual

VladRassokhin · 2018-10-08T15:44:38Z

src/kotlin/org/intellij/plugins/hcl/terraform/config/model/TypeModelLoader.kt

+    val defaultObj = value.obj("Default")
+    val defaultValue = if (defaultObj == null || defaultObj.isEmpty()) {
+      null
+    }


Please reformat code

VladRassokhin · 2018-10-08T15:46:27Z

src/kotlin/org/intellij/plugins/hcl/terraform/config/model/TypeModelLoader.kt

@@ -347,7 +348,24 @@ class TypeModelLoader(val external: Map<String, TypeModelProvider.Additional>) {
    val conflicts: List<String> = value.array<String>("ConflictsWith")?.map { it } ?: emptyList()

    val deprecated = value.string("Deprecated")
-    val has_default: Boolean = value.obj("Default")?.isNotEmpty() ?: false
+    val defaultObj = value.obj("Default")
+    val defaultValue = if (defaultObj == null || defaultObj.isEmpty()) {


Seems if could be simplified to just defaultObj?.let { ... } since there null -> null block in when which will sort out isEmpty case

VladRassokhin · 2018-10-08T15:47:09Z

src/kotlin/org/intellij/plugins/hcl/terraform/config/model/TypeModelLoader.kt

+        "Float", "TypeFloat" -> defaultValueString ?: "0"
+        "String", "TypeString" -> defaultValueString ?: ""
+        else -> {
+          LOG.warn("Unhandled default type $typeString for $fqn")


logErrorAndFailInInternalMode could be used where to detect unsupported types in tests

VladRassokhin · 2018-10-08T15:48:39Z

Thank you for contribution.

First, could you please sign JetBrains CLA

About PR itself I see there're some useless parts of shema being generated.

        "Default": {
          "Type": "string"
        }

Apart from that I'd like to have separate commit for code and shema update (or even no schema update since it's hard to navigate through PR diff and I'd anyway regenerate them just before releasing next version)

- Rename compare() -> isEquals() - Use logErrorAndFailInInternalMode() to report error Also changed "application" to private class property in TypeModelLoader to avoid passing it around lots of methods.

matthew16550 · 2018-10-10T13:58:29Z

schemas-extractor/template/generate-schema.go

+}
+
+func (e *SchemaElement) MarshalJSON() ([]byte, error) {
+	if e.Value == nil {


Maybe there is a better way? I don't know much about Go.

Neither do I :)

Origin

Value string `json:",omitempty"`

Should do the trick. You don't need to specify separate Marshaller for this.

And string in go is actually []byte which is actually already pointer type. And in most cases it is better to use just string instead of *string. And compare string with empty string ("") instead of null. What is literally the same.

I don't understand why original omitempty doesn't work.

matthew16550 · 2018-10-10T14:03:14Z

I've addressed all the PR comments.

The useless schema like below is because of json:",omitempty", I've put in a kludge to work around it.

"Default": {
  "Type": "string"
}

Add an inspection that reports properties being set to their default …

060121a

…value

matthew16550 commented Oct 8, 2018

View reviewed changes

VladRassokhin reviewed Oct 8, 2018

View reviewed changes

matthew16550 added 3 commits October 11, 2018 02:02

Also generate Default.Value when Value is the empty string.

6d6b155

Addressing PR comments:

073bba9

- Rename compare() -> isEquals() - Use logErrorAndFailInInternalMode() to report error Also changed "application" to private class property in TypeModelLoader to avoid passing it around lots of methods.

Remove schema files from PR

dd2edf8

matthew16550 commented Oct 10, 2018

View reviewed changes

Merge branch 'master' into inspect-property-defaults

038d4a5

VladRassokhin force-pushed the master branch from 6c7a2b3 to 0bd7ddd Compare December 17, 2019 10:35

matthew16550 closed this by deleting the head repository May 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an inspection that reports properties being set to their default value #169

Add an inspection that reports properties being set to their default value #169

matthew16550 commented Oct 8, 2018

matthew16550 commented Oct 8, 2018

matthew16550 Oct 8, 2018

matthew16550 Oct 8, 2018

VladRassokhin Oct 8, 2018

matthew16550 Oct 9, 2018

VladRassokhin Oct 8, 2018 •

edited

VladRassokhin Oct 8, 2018

VladRassokhin Oct 8, 2018

VladRassokhin Oct 8, 2018

VladRassokhin commented Oct 8, 2018

matthew16550 Oct 10, 2018

VladRassokhin Oct 13, 2018

werwolfby Dec 30, 2020

matthew16550 commented Oct 10, 2018

Add an inspection that reports properties being set to their default value #169

Add an inspection that reports properties being set to their default value #169

Conversation

matthew16550 commented Oct 8, 2018

matthew16550 commented Oct 8, 2018

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

VladRassokhin Oct 8, 2018 • edited

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

VladRassokhin commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew16550 commented Oct 10, 2018

VladRassokhin Oct 8, 2018 •

edited