Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: synchronize the inCodeValidator output to match the CLI -json #260

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

barrett-schonefeld
Copy link
Contributor

@barrett-schonefeld barrett-schonefeld commented Mar 8, 2021

Purpose:

  • Both the inCodeValidator and CLI -json return objects, so we want to standardize and keep these formats in sync.
  • Make output changes to fewer places when introducing new features. Simplify the code base.

Changes:

  • Add infos and hints to the JSON output.
  • Split the object creation and the printing of the JSON object into separate functions, so the inCodeValidator may utilize the same object-creation function.

Tests:

  • Add a test utility that aggregates the inCodeValidator errors, warnings, etc. into a single array to convert output to a format that is still compatible with existing tests.
  • Remove checks that expect the infos and hints fields to be undefined.

@dpopp07
Copy link
Member

dpopp07 commented Mar 9, 2021

@barrett-schonefeld I see you've included a "BREAKING CHANGE" notice in your commit message. We need to be careful here, because this will trigger a major release of the tool, aka 1.0.0. A formal "1.0" release is something that we want to plan out more and include more changes in, so I don't think we're ready for that. While your change here is "breaking", breaking changes are allowed in any 0.x version, so we don't need to make it formal. It can just be a feature release.

You will probably need to amend your commit and force push to make sure that line isn't included when you merge.

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay this is a good start. Certainly, that JSON handling should have been happening from one file from the beginning, so thanks for reorganizing.

I've left a few stylistic changes to make (hopefully they are not too picky) but I have a bigger concern to address.

You fixed the problem as stated in the issue but since you're changing this part of the code and it will be a breaking change already, I'm realizing we should rethink what this JSON output looks like. When I first wrote these components of the validator (probably back in 2017), I selfishly organized the results by filename so that it was easier to track down where bugs were. This was before we had "rule names" and "configuration". Now that we have them and generally know where rules are coming from, maybe we should reorganize it. Especially because the filenames are meaningless to the end user. When they use the --json flag and see

    "schema-ibm": [
      {
        "path": [
          "components",
          "schemas",
          "Pet",
          "properties",
          "photoUrls"
        ],
        "message": "Property names must follow case convention: lower_snake_case",
        "rule": "property_case_convention",
        "line": 178
      }
    ]

that "schema-ibm" key doesn't really do them any good and is kind of confusing. I don't necessarily have an alternate approach in mind, we can think of one together if you'd like, but this seems like as good of a chance as any to restructure the JSON results so that they are more user-friendly. Thoughts?

cc @mkistler

src/cli-validator/runValidator.js Outdated Show resolved Hide resolved
src/lib/index.js Outdated Show resolved Hide resolved
src/cli-validator/utils/jsonResults.js Outdated Show resolved Hide resolved
test/test-utils/aggregateInCodeResults.js Outdated Show resolved Hide resolved
test/cli-validator/tests/expected-output.test.js Outdated Show resolved Hide resolved
test/cli-validator/tests/expected-output.test.js Outdated Show resolved Hide resolved
Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Thanks for making these changes. Looks good to me 👍

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

I particularly like +91 -199. A cleaner, simpler design means less code to maintain.

…utput

Purpose:
- Both the inCodeValidator and CLI -json return objects, so we want to standardize and keep these formats in sync.
- Make output changes to fewer places when introducing new features. Simplify the code base.
- Improve the JSON output by removing the validator names from the JSON output.

Changes:
- Add `infos` and `hints` to the JSON output.
- Split the object creation and the printing of the JSON object into separate functions, so the `inCodeValidator` may utilize the same object-creation function.
- Reformat the JSON output. Remove the validator names from the JSON output.

Tests:
- Update tests that depend on the assumption that validator names are included in JSON output.
- Update tests that rely on empty `errors`, `warnings`, `infos`, `hints` objects when none of those errors occur. Updated tests to expect those objects to be undefined.
@barrett-schonefeld barrett-schonefeld merged commit e61f803 into main Mar 12, 2021
@barrett-schonefeld barrett-schonefeld deleted the standardize-output branch March 12, 2021 14:49
dpopp07 pushed a commit that referenced this pull request Mar 12, 2021
# [0.36.0](v0.35.2...v0.36.0) (2021-03-12)

### Features

* synchronize the inCodeValidator output to match the CLI -json output ([#260](#260)) ([e61f803](e61f803))
@dpopp07
Copy link
Member

dpopp07 commented Mar 12, 2021

🎉 This PR is included in version 0.36.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants