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.

Sign up for GitHub

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

Jump to bottom

feat: add --json to lint command #228

Merged
merged 14 commits into from
Mar 12, 2025
Merged

feat: add --json to lint command #228

merged 14 commits into from
Mar 12, 2025

Conversation

Karan-Palan
Copy link
Contributor

What kind of change does this PR introduce?

  • Feature: Added support for the --json (or -j) option in the lint command.

Issue Number:

Screenshots/videos:

image

If relevant, did you update the documentation?

Not yet

@Karan-Palan
Copy link
Contributor Author

Based on this discussion. @jviotti PTAL, if it looks fine, I'll add some tests and update the docs.

@jviotti
Copy link
Member

jviotti commented Feb 12, 2025

@Karan-Palan Great start! Some minor tweaks to be more JSON Schema idiomatic:

  • Can you rename passed to valid?
  • Can you rename issues to errors?
  • Can you rename file to path?
  • Can you rename rule to id?
  • Can you rename pointer to schemaLocation?

Overall, just trying to make it match JSON Schema Standard Output Formats a bit, at least in style!

jviotti
jviotti reviewed Feb 12, 2025
View reviewed changes
src/command_lint.cc Outdated
@@ -47,20 +49,46 @@ auto sourcemeta::jsonschema::cli::lint(
sourcemeta::core::schema_format_compare);
output << "\n";
}

// If --fix and --json were both requested, optionally print a small JSON:
Copy link
Member

Choose a reason for hiding this comment

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

In similar cases, other linters will output exactly the same, just that if you run it again, there will be no issues. Let's adopt that, as developers will be familiar with that

@jviotti
Copy link
Member

jviotti commented Feb 12, 2025

I also left a tiny comment, but looks pretty good. Looking forward to some tests and the docs

@Karan-Palan
Copy link
Contributor Author

I also left a tiny comment, but looks pretty good. Looking forward to some tests and the docs

Done, PTAL

jviotti
jviotti reviewed Feb 13, 2025
View reviewed changes
docs/lint.markdown Outdated
@@ -27,6 +27,8 @@ automatically fix many of them.

**The `--fix/-f` option is not supported when passing YAML schemas.**

**The `--json/-j` option prints results as a JSON object rather than human‐readable text.**
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this line. We don't have it for inspect (which also supports the same flag) and I guess it is pretty self explanatory

jviotti
jviotti reviewed Feb 13, 2025
View reviewed changes
test/lint/fail_lint_enum_with_type.sh Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This test is not exercising the --json flag? We have linting tests already. The point is to do linting with --json on the new ones!

jviotti
jviotti reviewed Feb 13, 2025
View reviewed changes
test/lint/pass_lint_fix_single_enum.sh Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Same here. This test is not exercising the --json flag.

@Karan-Palan Karan-Palan force-pushed the feat/add-json-output-to-lint branch from 15a2aae to 919197b Compare February 13, 2025 19:52
@Karan-Palan
Copy link
Contributor Author

Done, ptal

jviotti
jviotti reviewed Feb 13, 2025
View reviewed changes
src/command_lint.cc Outdated

if (output_json) {
auto msg = sourcemeta::core::JSON::make_object();
msg.assign("fixApplied", sourcemeta::core::JSON{true});
Copy link
Member

Choose a reason for hiding this comment

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

We don't want this. If --fix was passed, still print the normal JSON output

jviotti
jviotti reviewed Feb 13, 2025
View reviewed changes
src/main.cc Outdated
[--ignore/-i <schemas-or-directories>]

Lint the input schemas and potentially fix the reported issues.
The --fix/-f option is not supported when passing YAML schemas.
Use --json/-j to output lint errors and warnings in JSON format.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Use --json/-j to output lint errors and warnings in JSON format.
Use --json/-j to output lint errors in JSON.

@jviotti
Copy link
Member

jviotti commented Feb 13, 2025 •
edited
Loading

Looking good! Can you add one more tests exercising --fix alongside --json? The test should assert the output command and confirm by re-reading the schema that the lint issues have been fixed

@Karan-Palan
Copy link
Contributor Author

Done, ptal

jviotti
jviotti reviewed Feb 27, 2025
View reviewed changes
test/lint/pass_lint_fix_json.sh Outdated
{
"valid": true,
"errors": [],
"fixApplied": true
Copy link
Member

Choose a reason for hiding this comment

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

Let's get rid of this flag. We shouldn't report it. Usually what linters do with --fix is that they will still report ALL the errors that were fixed (as if you didn't pass --fix) but output code 0. And if you run it again, you get no errors reported at all

jviotti
jviotti reviewed Feb 27, 2025
View reviewed changes
test/lint/pass_lint_json.sh Outdated
EOF

"$1" lint "$TMP/schema.json" --json >"$TMP/output.json" 2>&1 && CODE="$?" || CODE="$?"
test "$CODE" = "0" || exit 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test "$CODE" = "0" || exit 1
test "$CODE" = "0"

You don't need the OR. The entire script should fail on any line failures

jviotti
jviotti reviewed Feb 27, 2025
View reviewed changes
test/lint/pass_lint_json.sh Outdated
cat << 'EOF' > "$TMP/schema.json"
{
"$schema": "http://json-schema.org/draft-04/schema#",
"const": "foo"
Copy link
Member

Choose a reason for hiding this comment

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

This schema is invalid. const is not present in Draft 4. That's a linter rule we should have in the future I guess. For now, can you make sure this schema is at least valid? i.e.

Suggested change
"const": "foo"
"enum": [ "foo" ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added those in my rules :D

jviotti
jviotti reviewed Feb 27, 2025
View reviewed changes
test/lint/pass_lint_json.sh
@@ -0,0 +1,26 @@
#!/bin/sh
set -o errexit
Copy link
Member

Choose a reason for hiding this comment

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

Can you leave a blank line after the hash bang, like other scripts? Same in the other ones you added here

jviotti
jviotti reviewed Feb 27, 2025
View reviewed changes
src/command_lint.cc Outdated
}
sourcemeta::core::prettify(output_json_object, std::cout);
std::cout << "\n";
} else {
Copy link
Member

Choose a reason for hiding this comment

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

No need to have an else clause that does nothing. Can you delete it?

jviotti
jviotti reviewed Feb 27, 2025
View reviewed changes
src/command_lint.cc Outdated
output_json_object.assign("valid", sourcemeta::core::JSON{result});
output_json_object.assign("errors", sourcemeta::core::JSON{errors_array});
if (fix) {
output_json_object.assign("fixApplied", sourcemeta::core::JSON{true});
Copy link
Member

Choose a reason for hiding this comment

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

See my comment below. We shouldn't be reporting this

@Karan-Palan
Copy link
Contributor Author

Done. Apologies for the extra commits for adding blanks after hashes, my formatter for bash files removes extra lines (below hash and last line)

@jviotti
Copy link
Member

jviotti commented Mar 12, 2025

I left a few minor remaining suggestions, but its almost there!

@jviotti
Copy link
Member

jviotti commented Mar 12, 2025

Actually, I guess I can just accept them myself and just merge. Let me see

@jviotti
Apply suggestions from code review
8ef07bb 
Signed-off-by: Juan Cruz Viotti <[email protected]>
@jviotti
Apply suggestions from code review
6174e7d 
Signed-off-by: Juan Cruz Viotti <[email protected]>
@Karan-Palan
Copy link
Contributor Author

Thanks for your patience, @jviotti i! I really appreciate the feedback. I'll ensure that my future test cases follow the same format.

@jviotti jviotti merged commit 1c0323d into sourcemeta:main Mar 12, 2025
7 checks passed
@jviotti
Copy link
Member

jviotti commented Mar 12, 2025

Thank a lot for the work! I'll release a new version of the CLI in a bit

@jviotti
Copy link
Member

jviotti commented Mar 12, 2025

Done! See v7.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jviotti jviotti jviotti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Karan-Palan @jviotti