jeremiah-c-leary / vhdl-style-guide

Style guide enforcement for VHDL
GNU General Public License v3.0
175 stars 38 forks source link

Configuration JSON format is not JSON compliant #1145

Closed alonbl closed 2 months ago

alonbl commented 3 months ago

Hi,

Remaining issue from #1141.

  {
    "file_list": [
     "tests/vsg/read_configuration_files/entity.vhd",
      "tests/vsg/read_configuration_files/arch.vhd": {          <---- json cannot have dict key in a list
        "rule": {
          "rule_001": {
            "disable": true
          }
        }
      },
     "tests/vsg/read_configuration_files/package.vhd"
    ]
  }

I suggest the following:

if the value is string it is a name, if it is a dictionary take name from name key:

  {
    "file_list": [
     "tests/vsg/read_configuration_files/entity.vhd",
      {
        "name": "tests/vsg/read_configuration_files/arch.vhd",
        "rule": {
          "rule_001": {
            "disable": true
          }
        }
      },
     "tests/vsg/read_configuration_files/package.vhd"
    ]
  }
jeremiah-c-leary commented 2 months ago

Evening @alonbl ,

I reviewed the JSON standard and found out that a list can contain a string, number, object, array, true, false, or null. I am trying to define an object but I am missing the opening and closing curly braces. So the above file can be updated to this:

  {
    "file_list": [
      "tests/vsg/read_configuration_files/entity.vhd",
      {
        "tests/vsg/read_configuration_files/arch.vhd": {
          "rule": {
            "rule_001": {
              "disable": true
            }
          }
        }
      },
      "tests/vsg/read_configuration_files/package.vhd"
    ]
  }

I updated the two JSON files that were failing pre-commit and updated the documentation to include the curly braces.

I pushed an update to the issue-1145 branch. When you get a chance could you check it on your end.

Thanks,

--Jeremy

alonbl commented 2 months ago

Hi @jeremiah-c-leary ,

It is nice that this notation is supported without a code change :) And that we are not compliant with json.

However, I think the format is confusing.

Current notation, assumes generic key name (aka file) within a single dictionary.

{
    "file_list": [
        {
            "file1": {
                "key": "value"
            }
        },
        {
            "file2": {
                "key": "value"
            }
        },
        "file3"
    ]
}

Alternative:

{
    "file_list": {
        "file1": {
            "key": "value"
        },
        "file2": {
            "key": "value"
        },
        "file3"
    }
}

The alternative is simpler, however, the order of a map is not fixed, I believe that in this case the order matters.

In order to have proper schema, I suggested the following, as each item in the list is well defined:

{
    "file_list": [
        {
            "name": "file1",
            "rule": {
                "key": "value"
            }
        },
        {
            "name": "file2",
            "rule": {
                "key": "value"
            }
        },
        "file3"
    ]
}

Also in yaml, as the key names are not wildcards no need to play with yaml magic:

file_list:
   - name: file1/**
     rule:
       key: value
   - name: file2
     rule:
       key: value
   - file3

The interesting variation of the above is the ability to provide multiple names, which is impossible in the current one.

{
    "file_list": [
        {
            "name": [
                "file1/**",
                "file1.1/**"
            ],
            "rule": {
                "key": "value"
            }
        },
        {
            "name": "file2",
            "rule": {
                "key": "value"
            }
        },
        "file3"
    ]
}
file_list:
   - name:
     - file1/**
     - file1.1/**
     rule:
       key: value
   - name: file2
     rule:
       key: value
   - file3

Branch issue-1145 is great in term of making json pass, just wanted to write why the current notation is a bit complex and limiting.

Regards,

jeremiah-c-leary commented 2 months ago

Morning @alonbl ,

Would you be okay with merging this issue to resolve the error and create another one to discuss changes to configurations? There may be other improvements that could be made.

--Jeremy

alonbl commented 2 months ago

Sure. It solves the incompatibility. Thank you.

On Fri, 12 Apr 2024 at 15:16 Jeremiah Leary @.***> wrote:

Morning @alonbl https://github.com/alonbl ,

Would you be okay with merging this issue to resolve the error and create another one to discuss changes to configurations? There may be other improvements that could be made.

--Jeremy

— Reply to this email directly, view it on GitHub https://github.com/jeremiah-c-leary/vhdl-style-guide/issues/1145#issuecomment-2051648341, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJURLJW7KKNDSFNVCRAY6LY47GCTAVCNFSM6AAAAABFFQC2VCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJRGY2DQMZUGE . You are receiving this because you were mentioned.Message ID: @.***>

jeremiah-c-leary commented 2 months ago

Sweet, I will merge this to master.

--Jeremy