[Feature Request] Consistent handling of qmk format-XXX with inplace rewrites and printing

[tzarc]

Feature Request Type

• Core functionality
• Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
• Alteration (enhancement/optimization) of existing feature(s)
• New behavior

Description

#21610 shows that we have some inconsistency with all the file formatters, either printing, not printing, replacing file contents automatically etc. etc.

This issue is to track some discussion on making them all consistent.

Keep in mind that some of these formatters are used as part of CI/CD linting and the like, so any proposed changes need to also take into account any required changes in the workflows as well.

[dexter93]

current setup comparison:

python

@cli.argument('-n', '--dry-run', arg_only=True, action='store_true', help="Don't actually format.")
@cli.argument('-b', '--base-branch', default='origin/master', help='Branch to compare to diffs to.')
@cli.argument('-a', '--all-files', arg_only=True, action='store_true', help='Format all files.')
@cli.argument('files', nargs='*', arg_only=True, type=normpath, help='Filename(s) to format.')

c

@cli.argument('-n', '--dry-run', arg_only=True, action='store_true', help="Flag only, don't automatically format.")
@cli.argument('-b', '--base-branch', default='origin/master', help='Branch to compare to diffs to.')
@cli.argument('-a', '--all-files', arg_only=True, action='store_true', help='Format all core files.')
@cli.argument('--core-only', arg_only=True, action='store_true', help='Format core files only.')
@cli.argument('files', nargs='*', arg_only=True, type=normpath, completer=FilesCompleter('.c'), help='Filename(s) to format.')

json

@cli.argument('json_file', arg_only=True, type=normpath, help='JSON file to format')
@cli.argument('-f', '--format', choices=['auto', 'keyboard', 'keymap'], default='auto', arg_only=True, help='JSON formatter to use (Default: autodetect)')

text

@cli.argument('-b', '--base-branch', default='origin/master', help='Branch to compare to diffs to.')
@cli.argument('-a', '--all-files', arg_only=True, action='store_true', help='Format all files.')
@cli.argument('files', nargs='*', arg_only=True, type=normpath, help='Filename(s) to format.')

/******************************************************************************/

Least effort/common standardization proposal
-n: --dry-run : flag offending files
-b: --base-branch : base branch selection
-a: --all-files : format all files
-c: --core-only : format core files only
-f: --files : format specified files only

Proposed functions
-h: --help : print all supported flags/functions to STDOUT
-i : --inline : format and replace file inline, print suceess/fail status to STDOUT
-v : --verbose : print diff in STDOUT
-p : --parser : specify parser/formatter version to use ( probably internal use - only to json currently)

Suggested defaults

• if file is detected in prompt: -if
• if file is not detected in prompt: -ia

This should probably cover all functionality - covering use cases for human or CI invocation.

Additionally, maybe consider a wrapper qmk format that autodetects file types, and selects the appropriate tool, forwarding the relevant flags to it.

[elpekenin]

Should be rather easy to standarize by having a wrapper for the common flags, eg:

def qmk_formatter(func):
    """Base flags for all formatters (json, c, py, etc)"""

    @cli.argument(...)
    @cli.argument(...)
    ...
    @wraps(func)
    def wrapper(cli):
        func(cli)

    return wrapper

Then specific implementations could use this, and perhaps add extra specific config on top

@cli.arg(...)
@qmk_formatter
def a_text_formatter(cli):
    ...

Could probably move part of the logic to the wrapper, eg dumping to file/stdout

# modifying the 1st block of code
def wrapper(cli):
    output = func(cli)
    if cli.args.inplace:
        ...

    if cli.args.print:
        print(output)

[tzarc]

Yeah makes sense.

For my preference, I think behaviour-wise I'd like it to match clang-format -- print to stdout unless -i/--inplace is specified. Mainly for consistency with other tooling in the industry.

[elpekenin]

Will try and PR later today

[elpekenin]

Initial (WIP) code can be found here: elpekenin@83f1a90#diff-9e5fbad4b34e52b55c652084d557b379ba22474f8d3d94f3e67ffde2f636e029R16

Any feedback is very welcome :)

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 30 days. If this issue is still valid, re-open the issue and let us know.
// [stale-action-closed]