feat: add json formatter #336

Kashugoyal · 2025-10-09T19:39:22Z

Resolves #159

This PR:

Adds a JSON formatter to format JSON files
- Uses the package https://github.com/tidwall/pretty/tree/master for JSON formatting
Adds the CLI and env flags for enable the formatter
Adds and updates relevant unit and functional tests
Updates existing JSON files in the repository to match the format

Left for Future work:

Support for formatting check of other file types
Allow configuration for formatters. For example allow single vs double indent

kehoecj · 2025-10-10T16:57:38Z

@Kashugoyal Thanks for the PR - please address all Golang-CI comments and fix failing unit tests

Kashugoyal · 2025-10-15T00:57:59Z

@Kashugoyal Thanks for the PR - please address all Golang-CI comments and fix failing unit tests

@kehoecj PR is ready for review.

kehoecj

Works as advertised but needs some architectural changes

kehoecj · 2025-10-15T17:43:47Z

cmd/validator/validator.go

 		groupOutputPtr      = flag.String("groupby", "", "Group output by filetype, directory, pass-fail. Supported for Standard and JSON reports")
 		quietPtr            = flag.Bool("quiet", false, "If quiet flag is set. It doesn't print any output to stdout.")
 		globbingPrt         = flag.Bool("globbing", false, "If globbing flag is set, check for glob patterns in the arguments.")
+		formatPtr           = flag.Bool("format", false, "Format all supported file types in place. Only json is supported currently.")


Thinking ahead to when multiple types are supported:

Users need to be able to specify which config types they want to format. All can be the default but needs to be configurable

Need to think about how we'll allow users to specify formatting settings. I know you had identified it in future work but I think it needs to be part of this PR. I don't think we'll be able to specify this on the command line, will probably have to be a config file (or multiple config files)

Can we do flags like --format-all, --format-json, --format-yaml, etc?
format-xxx to enable formatter for a specific file type,
format-all for all
format-config to follow config specification, e.g. --format-config ./formatter_config.json

For the settings specification, it can be a JSON file with top level keys for file types.
e.g.:

"formatters": ["json", "yaml", ..], "json": { "indent": 2}, "toml": {..} ...

The implementation for the config would be a larger change and maybe we can address as a separate PR.

What about passing options to -format? Example: -format=all OR -format=json,yaml,ini?

I like the settings JSON - that makes sense to me. Agree that would be part of a larger change later, just wanted to ensure the initial design didn't constrain the later implementation

kehoecj · 2025-10-15T18:01:28Z

pkg/cli/cli.go

+	if ShouldFormat {
+		if exitCode, err := formatFiles(filesToFormat); err != nil {
+			return exitCode, fmt.Errorf("unable to format files: %w", err)
+		}
+	}


The design of this could be refactored to be more efficient:

Call the formatter in the initial foundFiles loop rather than adding it to a list then looping again.

The formatFiles function logic should be moved down into the formatter rather than in the CLI

I made the changes as requested. Please take another look.

go.mod

Kashugoyal · 2025-10-18T05:46:16Z

@ccoVeille PR is ready for another look

ccoVeille · 2025-10-18T16:41:51Z

Instead of reviewing the PR, I have just shared my thoughts and concerns on the issue that this PR is about.

#159 (comment)

Your code is OK. I could review it. But for now, I prefered to step out.

Note to make it clear I'm not against what you coded or the way you coded, I just feel it shouldn't be added to the tool

kehoecj · 2025-10-22T13:53:14Z

@Kashugoyal Please make the following changes to the implementation based off the discussion in #159 :

Change the CLI --format flag to --check-format to be more explicit that it will simply validate the formatting against the defaults, not make any changes. Users should be able to scope this to list of config types, i.e. --check-format=json,yaml
Passing --check-format should only validate the formatting, not make the formatting changes. That will be handled by a --fix flag that will be implemented later
Add new code under pkg/validator to validate the formatting against the defaults if --check-format is passed

Let me know if that makes sense.

Kashugoyal · 2025-10-24T02:02:36Z

@Kashugoyal Please make the following changes to the implementation based off the discussion in #159 :

Change the CLI --format flag to --check-format to be more explicit that it will simply validate the formatting against the defaults, not make any changes. Users should be able to scope this to list of config types, i.e. --check-format=json,yaml

Passing --check-format should only validate the formatting, not make the formatting changes. That will be handled by a --fix flag that will be implemented later

Add new code under pkg/validator to validate the formatting against the defaults if --check-format is passed

Let me know if that makes sense.

@kehoecj what design do you prefer? I can list a few options below:

Add another method to the Validator interface.

type Validator interface {
     Validate(b []byte) (bool, error)
     ValidateFormat(b []byte, options interface{}) (bool, error)
}

options is a future looking feature. Can be skipped for the first iteration.

Add a parameter to the existing method

type Validator interface {
     Validate(b []byte, checkFormat bool) (bool, error)
}

kehoecj · 2025-10-24T02:23:37Z

@Kashugoyal Please make the following changes to the implementation based off the discussion in #159 :

Change the CLI --format flag to --check-format to be more explicit that it will simply validate the formatting against the defaults, not make any changes. Users should be able to scope this to list of config types, i.e. --check-format=json,yaml

Passing --check-format should only validate the formatting, not make the formatting changes. That will be handled by a --fix flag that will be implemented later

Add new code under pkg/validator to validate the formatting against the defaults if --check-format is passed

Let me know if that makes sense.

@kehoecj what design do you prefer? I can list a few options below:
Add another method to the Validator interface.
type Validator interface {
     Validate(b []byte) (bool, error)
     ValidateFormat(b []byte, options interface{}) (bool, error)
}
options is a future looking feature. Can be skipped for the first iteration.
Add a parameter to the existing method
type Validator interface {
     Validate(b []byte, checkFormat bool) (bool, error)
}

Good timing, I am going through the same design decision now that I'm adding schema. I was leaning toward #1 but the downside is that the interface is now more complicated. I don't like #2 because it makes the interface definition more complicated and forces design decisions into the implementing functions.

I think I'm leaning toward interface composition:

type SyntaxValidator interface {
  ValidateSyntax(b []byte) (bool, error)
}

type FormatValidator interface {
  ValidateFormat(Validate(b []byte, options interface{}) (bool, error)
}

// coming in my Sarif PR
type SchemaValidator interface {
  ValidateSchema(Validate(b []byte, schemaDefinition interface{}) (bool, error)
}

type Validator interface {
  SyntaxValidator
  FormatValidator
  SchemaValidator
}

That way, when required, the different validator interfaces can be fulfilled and referenced. What do you think?

Kashugoyal · 2025-10-24T02:28:40Z

@Kashugoyal Please make the following changes to the implementation based off the discussion in #159 :

Change the CLI --format flag to --check-format to be more explicit that it will simply validate the formatting against the defaults, not make any changes. Users should be able to scope this to list of config types, i.e. --check-format=json,yaml

Passing --check-format should only validate the formatting, not make the formatting changes. That will be handled by a --fix flag that will be implemented later

Add new code under pkg/validator to validate the formatting against the defaults if --check-format is passed

Let me know if that makes sense.

@kehoecj what design do you prefer? I can list a few options below:
Add another method to the Validator interface.
type Validator interface {
     Validate(b []byte) (bool, error)
     ValidateFormat(b []byte, options interface{}) (bool, error)
}
options is a future looking feature. Can be skipped for the first iteration.
Add a parameter to the existing method
type Validator interface {
     Validate(b []byte, checkFormat bool) (bool, error)
}
Good timing, I am going through the same design decision now that I'm adding schema. I was leaning toward #1 but the downside is that the interface is now more complicated. I don't like #2 because it makes the interface definition more complicated and forces design decisions into the implementing functions.

I think I'm leaning toward interface composition:
type SyntaxValidator interface {
  ValidateSyntax(b []byte) (bool, error)
}

type FormatValidator interface {
  ValidateFormat(Validate(b []byte, options interface{}) (bool, error)
}

// coming in my Sarif PR
type SchemaValidator interface {
  ValidateSchema(Validate(b []byte, schemaDefinition interface{}) (bool, error)
}

type Validator interface {
  SyntaxValidator
  FormatValidator
  SchemaValidator
}
That way, when required, the different validator interfaces can be fulfilled and referenced. What do you think?

I like your idea. Will do that!

…to json-formatter

Kashugoyal · 2025-10-28T05:54:07Z

@kehoecj marking the MR ready for review. It has been a lot of back and forth therefore I may have missed some things in my MR, please review 😄

kehoecj

During functional testing the following issue was found:

When passing the --check-format, the provided path is not honored by the confi-file-validator tool and it instead scans the current working directory:

~/src/github.com/boeing/pr/formatting/config-file-validator$ ./validator --check-for
mat test/fixtures/bad_format.json 
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/dependabot.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/changelog.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/go.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/golangci-lint.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/goreportcard.yaml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/mega-linter.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/release.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/scorecard.yml

When I remove the --check-format flag, the tools works as expected

…to json-formatter

Kashugoyal · 2025-11-19T03:15:02Z

During functional testing the following issue was found:

When passing the --check-format, the provided path is not honored by the confi-file-validator tool and it instead scans the current working directory:

~/src/github.com/boeing/pr/formatting/config-file-validator$ ./validator --check-for
mat test/fixtures/bad_format.json 
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/dependabot.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/changelog.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/go.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/golangci-lint.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/goreportcard.yaml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/mega-linter.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/release.yml
    ✓ /src/github.com/boeing/pr/formatting/config-file-validator/.github/workflows/scorecard.yml

When I remove the --check-format flag, the tools works as expected

In the above case, the check-format flag assumes the following string is the file type and not the path. I added a validation function for this case and also added a test case for this scenario.

chore: Updated coverage badge.

f3f3e93

Kashugoyal changed the title ~~feat: add json formatter~~ draft: feat: add json formatter Oct 9, 2025

kehoecj added OSS Community Contribution Contributions from the OSS Community waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 10, 2025

kehoecj self-requested a review October 10, 2025 14:27

kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 10, 2025

Kashugoyal marked this pull request as draft October 10, 2025 17:02

Kashugoyal force-pushed the json-formatter branch 2 times, most recently from dcf1526 to 2061a41 Compare October 15, 2025 00:18

Kashugoyal marked this pull request as ready for review October 15, 2025 00:57

kehoecj changed the title ~~draft: feat: add json formatter~~ feat: add json formatter Oct 15, 2025

kehoecj added waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers and removed pr-action-requested PR is awaiting feedback from the submitting developer labels Oct 15, 2025

kehoecj requested changes Oct 15, 2025

View reviewed changes

kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Oct 15, 2025

Kashugoyal marked this pull request as draft October 17, 2025 23:59

Kashugoyal force-pushed the json-formatter branch from 3bc273e to 81b0d3a Compare October 18, 2025 05:40

Kashugoyal marked this pull request as ready for review October 18, 2025 05:45

kehoecj added the hacktoberfest-accepted Valid PR Hacktoberfest PR label Oct 23, 2025

Kashugoyal marked this pull request as draft October 24, 2025 03:59

Merge branch 'Boeing:main' into main

b449174

Kashugoyal force-pushed the json-formatter branch from f367ea7 to ff68e7d Compare October 28, 2025 05:32

Kashugoyal and others added 2 commits October 28, 2025 05:33

Merge branch 'Boeing:main' into main

bf5289c

feat: add json files format check

4f986c7

Kashugoyal force-pushed the json-formatter branch from a0c1e76 to 4f986c7 Compare October 28, 2025 05:46

Merge branch 'main' of github.com:Kashugoyal/config-file-validator in…

d263e21

…to json-formatter

Kashugoyal marked this pull request as ready for review October 28, 2025 05:53

Kashugoyal requested a review from a team as a code owner October 28, 2025 05:53

kehoecj added waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers and removed pr-action-requested PR is awaiting feedback from the submitting developer labels Oct 28, 2025

kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Nov 17, 2025

kehoecj requested changes Nov 17, 2025

View reviewed changes

Kashugoyal and others added 4 commits November 18, 2025 21:39

fix: validation for check format flag

0c4a80e

Merge branch 'Boeing:main' into main

959b8bf

Merge branch 'main' of github.com:Kashugoyal/config-file-validator in…

ebdb496

…to json-formatter

Merge branch 'Boeing:main' into main

b2d9a22

Kashugoyal marked this pull request as draft November 19, 2025 02:51

Kashugoyal marked this pull request as ready for review November 19, 2025 03:13

kehoecj added waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers and removed pr-action-requested PR is awaiting feedback from the submitting developer labels Nov 19, 2025

feat: add json formatter #336

Are you sure you want to change the base?

feat: add json formatter #336

Uh oh!

Conversation

Kashugoyal commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kehoecj commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kashugoyal commented Oct 15, 2025

Uh oh!

kehoecj left a comment

Choose a reason for hiding this comment

Uh oh!

kehoecj Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Kashugoyal Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

kehoecj Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

kehoecj Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

kehoecj Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Kashugoyal Oct 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kashugoyal commented Oct 18, 2025

Uh oh!

ccoVeille commented Oct 18, 2025

Uh oh!

kehoecj commented Oct 22, 2025

Uh oh!

Kashugoyal commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kehoecj commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kashugoyal commented Oct 24, 2025

Uh oh!

Kashugoyal commented Oct 28, 2025

Uh oh!

kehoecj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kashugoyal commented Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Kashugoyal commented Oct 9, 2025 •

edited

Loading

kehoecj commented Oct 10, 2025 •

edited

Loading

Kashugoyal commented Oct 24, 2025 •

edited

Loading

kehoecj commented Oct 24, 2025 •

edited

Loading

kehoecj left a comment •

edited

Loading