Add support for ktfmt by 0x26res · Pull Request #196 · macisamuele/language-formatters-pre-commit-hooks

0x26res · 2023-11-29T19:25:06Z

Here's the help for ktfmt:

$ java -jar ./ktfmt-0.46-jar-with-dependencies.jar  --help
Usage: ktfmt [--dropbox-style | --google-style | --kotlinlang-style] [--dry-run] [--set-exit-if-changed] [--stdin-name=<name>] [--do-not-remove-unused-imports] File1.kt File2.kt ...

Some notes:

nitpick: should we use consistent case for ktlint all lower case. I see KTLint/KTlint is used in places, but I don't see it spelled this way in the official repo. Also it seems ktfmt is always spelled in lower case.
ktfmt autofix by default, but has a --dry-run mode which I use by default (and turn off with --autofix
ktfmt also needs --set-exit-if-changed to be turned on so we get consistent error code (fail if a file is badly formatted)
I'm getting the ktfmt jar from maven, I couldn't find it on github
we should run black on this repo to format the code. I find it ironic that its code is not formatted (maybe in another PR).
great test, it took me some time to wrap my head around them, but then it's easy to add to them.

macisamuele

Thanks a lot for the contributions.

There are a few comments, mostly related to organisation and lints.
Hopefully it won't be a too big of an ask

macisamuele · 2023-12-03T11:44:16Z


+### How to use ktfmt instead of ktlint
+
+⚠: [ktfmt](https://github.com/facebook/ktfmt) is a more opinionated linter than [ktlin](https://github.com/pinterest/ktlint). It will change a lot of files


I would prefer to have no strong opinions on the tools offered in the documentation.

The main suggestions are:

remove the warning sign

remove the "It will change a lot of files"

Removed, I've just put the available format instead (google/dropbox/kotlinlang).

macisamuele · 2023-12-03T11:47:11Z

-
-    ktlint_jar = _download_kotlin_formatter_jar(
-        args.ktlint_version,
+    print(args)


Please remove the print of the arguments (possibly a manual testing leftover)

macisamuele · 2023-12-03T11:48:29Z

        help="KTLint version to use (default %(default)s)",
    )
-
+    parser.add_argument(


Need to think a little bit about the arguments, the PR adds 2 arguments that would only be honoured if ktfmt flag is passed.
As of now I don't have a much better option, hence not a blocker at all.

Agreed. I guess what's important is that autofix works in the same way for both ktlint and ktfmt. I'm not sure we can do much about therest.

macisamuele · 2023-12-03T11:53:14Z

+        ktfmt_args.append(f"--{ktfmt_style}-style")
+    if not autofix:
+        ktfmt_args.append("--dry-run")
+    filenames = filenames if filenames else ["./"]


pre-commit hooks will always pass the filenames.
We should NOT assume the all current directory.
Please do not tamper with filenames content

Furthermore, an empty iterable (depending on the actual type) might or might now be evaluated as False

>>> from typing import Iterable >>> list = [] >>> isinstance(list, Iterable), bool(list) (True, False) >>> isinstance(list, Iterable) True >>> bool(list) False >>> isinstance(iter(list), Iterable) True >>> bool(iter(list)) True >>>

macisamuele · 2023-12-03T11:56:27Z

    print(
        "[cwd={cwd}] Run command: {command}".format(
-            command=command,
+            command=' '.join(command),


As far as this print is present for debugging information to re-run the command, there is no big concern on joining the command.

Still I would suggest not to apply this change because assuming that the input filenames have spaces then "copy-pasting" the command would not work.

Makes sense, I've rolled back.

macisamuele · 2023-12-03T12:00:04Z

    method: typing.Callable[[typing.List[str]], int],
    not_pretty_formatted_path: str,
    formatted_path: str,
+    extra_parameters: list[str],


Adding this parameter is changing the function signature and effectively breaking tests.
Please consider the following changes

Suggested change

extra_parameters: list[str],

extra_parameters: typing.Optional[typing.List[str]] = None,

and while using extra_parameters doing something like (extra_parameters or [])

macisamuele · 2023-12-03T12:01:03Z

    copyfile(not_pretty_formatted_path, not_pretty_formatted_tmp_path)
    with change_dir_context(tmpdir.strpath):
-        parameters = ["--autofix", not_pretty_formatted_tmp_strpath]
+        parameters = extra_parameters + [not_pretty_formatted_tmp_strpath]


This test is specific to run autofix tests, the --autofix parameter has to be present.
Please do not remove the "default" injection of the parameter

Suggested change

parameters = extra_parameters + [not_pretty_formatted_tmp_strpath]

parameters = (extra_parameters or []) + ["--autofix", not_pretty_formatted_tmp_strpath]

macisamuele · 2023-12-03T12:01:35Z

    # file was formatted (shouldn't trigger linter again)
    with change_dir_context(tmpdir.strpath):
-        parameters = ["--autofix", not_pretty_formatted_tmp_strpath]
+        parameters = extra_parameters + [not_pretty_formatted_tmp_strpath]


as above

Suggested change

parameters = extra_parameters + [not_pretty_formatted_tmp_strpath]

parameters = (extra_parameters or []) + ["--autofix", not_pretty_formatted_tmp_strpath]

macisamuele · 2023-12-03T12:03:55Z

+from language_formatters_pre_commit_hooks.pretty_format_kotlin import \
+    _download_ktlint_formatter_jar, _download_ktfmt_formatter_jar


Most likely you want to run pre-commit hooks on your PR.

You would have test failure otherwise.
If you don't want to install the pre-commit hooks of the repository then please run tox -e pre-commit

macisamuele · 2023-12-03T12:07:21Z

+def test_pretty_format_kotlin(undecorate_method, filename, expected_retval, extra_parameters):
+    assert undecorate_method(extra_parameters + [filename]) == expected_retval


It might be worth splitting this test in

test_pretty_format_kotlin_ktlint (previous test)

test_pretty_format_kotlin_ktfmt

test_pretty_format_kotlin_ktfmt_with_dropbox_style

...

Tests are going to be somewhat easier to handle and more importantly you won't need to pass --ktfmt argument all the times and for the handled styles then --ktfmt-style=... is not needed (as the test can add it)

Done, I agree this was a mouthful

0x26res · 2023-12-04T09:30:50Z

Thanks for the feedback and spending the time to explain everything.

👍

macisamuele

Thanks a lot for your changes.
Really appreciated the effort and the final results 😊

codecov · 2023-12-15T21:02:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (23ad93b) 100.00% compared to head (9c2d33a) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #196   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          317       333   +16     
=========================================
+ Hits           317       333   +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

0x26res · 2023-12-17T20:39:04Z

@macisamuele I was trying to test locally and realized I was missing a data file. See my PR: #199

Summary: I've recently added support for ktfmt to pre-commit hooks. macisamuele/language-formatters-pre-commit-hooks#196 I thought it'd be worth mentioning in the README. Pull Request resolved: #462 Reviewed By: cortinico Differential Revision: D56680818 Pulled By: hick209 fbshipit-source-id: 3d6482e3d550b5d04647d6582ac3348aac52f9c4

aandres added 5 commits November 29, 2023 17:52

Working ish version

932f9b7

Rollback changes to do with autofix and dry run

106dcde

update doc

4bdaf67

Wrap up

3095389

Fix doc

d1bf17d

macisamuele requested changes Dec 3, 2023

View reviewed changes

MR feedback

43ef761

Merge branch 'master' into 195-add-support-for-ktfmt

889525a

macisamuele approved these changes Dec 15, 2023

View reviewed changes

macisamuele enabled auto-merge December 15, 2023 20:23

Ignore coverage regression while fetching version for not existing tool

9c2d33a

macisamuele force-pushed the 195-add-support-for-ktfmt branch from 14cd9f4 to 9c2d33a Compare December 15, 2023 20:57

macisamuele merged commit 44e4c2f into macisamuele:master Dec 15, 2023

0x26res mentioned this pull request Feb 22, 2024

Feature: IDEA: trigger ktfmt check on attempted commit facebook/ktfmt#255

Closed

mxr mentioned this pull request Apr 4, 2024

Add support for ktfmt #195

Closed

0x26res mentioned this pull request Apr 28, 2024

Add pre commit hooks to readme facebook/ktfmt#462

Closed


		### How to use ktfmt instead of ktlint

		⚠: [ktfmt](https://github.com/facebook/ktfmt) is a more opinionated linter than [ktlin](https://github.com/pinterest/ktlint). It will change a lot of files

	extra_parameters: list[str],
	extra_parameters: typing.Optional[typing.List[str]] = None,

	parameters = extra_parameters + [not_pretty_formatted_tmp_strpath]
	parameters = (extra_parameters or []) + ["--autofix", not_pretty_formatted_tmp_strpath]

		from language_formatters_pre_commit_hooks.pretty_format_kotlin import \
		_download_ktlint_formatter_jar, _download_ktfmt_formatter_jar

		def test_pretty_format_kotlin(undecorate_method, filename, expected_retval, extra_parameters):
		assert undecorate_method(extra_parameters + [filename]) == expected_retval

Conversation

0x26res commented Nov 29, 2023

Some notes:

Uh oh!

macisamuele left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x26res commented Dec 4, 2023

Uh oh!

macisamuele left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Dec 15, 2023

Codecov Report

Uh oh!

0x26res commented Dec 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants