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

Add support for ktfmt #196

Merged
merged 8 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-hooks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
minimum_pre_commit_version: '1'
- id: pretty-format-kotlin
name: KTLint
description: Runs KTLint over Kotlin source files
description: Runs KTLint (or ktfmt) over Kotlin source files
entry: pretty-format-kotlin
language: python
types: [kotlin]
Expand Down
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,18 @@ You can pass the jar file path to `--google-java-formatter-jar` argument:
args: [--google-java-formatter-jar=/usr/bin/google-java-format-1.17.0-all-deps.jar]
```

### 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
Copy link
Owner

Choose a reason for hiding this comment

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

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"

Copy link
Author

Choose a reason for hiding this comment

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

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


```yaml
- repo: https://github.com/macisamuele/language-formatters-pre-commit-hooks
rev: ...
hooks:
- id: pretty-format-kotlin
args: [--ktfmt --ktfmt-style=google]
```

## License

`language-formatters-pre-commit-hooks` is licensed with [`Apache License version 2.0`](http://www.apache.org/licenses/LICENSE-2.0.html).
1 change: 1 addition & 0 deletions language_formatters_pre_commit_hooks/ktfmt.version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0.46
69 changes: 61 additions & 8 deletions language_formatters_pre_commit_hooks/pretty_format_kotlin.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from language_formatters_pre_commit_hooks.utils import run_command


def _download_kotlin_formatter_jar(version: str) -> str: # pragma: no cover
def _download_ktlint_formatter_jar(version: str) -> str: # pragma: no cover
def get_url(_version: str) -> str:
# Links extracted from https://github.com/pinterest/ktlint/
return "https://github.com/pinterest/ktlint/releases/download/{version}/ktlint".format(
Expand All @@ -30,6 +30,18 @@ def get_url(_version: str) -> str:
)


def _download_ktfmt_formatter_jar(version: str) -> str: # pragma: no cover
url ="https://repo1.maven.org/maven2/com/facebook/ktfmt/{version}/ktfmt-{version}-jar-with-dependencies.jar".format(
version=version,
)
try:
return download_url(url)
except: # noqa: E722 (allow usage of bare 'except')
raise RuntimeError(
f"Failed to download {url}. Probably the requested version, {version}"
", is not valid or you have some network issue."
)

def _fix_paths(paths: typing.Iterable[str]) -> typing.Iterable[str]:
# Starting from KTLint 0.41.0 paths cannot contain backward slashes as path separator
# Odd enough the error messages reported by KTLint contain `\` :(
Expand All @@ -52,14 +64,55 @@ def pretty_format_kotlin(argv: typing.Optional[typing.List[str]] = None) -> int:
default=_get_default_version("ktlint"),
help="KTLint version to use (default %(default)s)",
)

parser.add_argument(
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

"--ktfmt-version",
dest="kftmt_version",
default=_get_default_version("ktfmt"),
help="ktfmt version to use (default %(default)s)",
)
parser.add_argument(
"--ktfmt",
action="store_true",
dest="ktfmt",
help="Use ktfmt",
)
parser.add_argument(
"--ktfmt-style",
choices=['dropbox', 'google', 'kotlinlang'],
dest="ktfmt_style",
help="Which style to use",
)
parser.add_argument("filenames", nargs="*", help="Filenames to fix")
args = parser.parse_args(argv)

ktlint_jar = _download_kotlin_formatter_jar(
args.ktlint_version,
print(args)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

removed

if args.ktfmt:
return run_ktfmt(args.kftmt_version, args.filenames, args.ktfmt_style, args.autofix)
else:
return run_ktlint(args.ktlint_version,args.filenames, args.autofix)


def run_ktfmt(ktfmt_version: str, filenames: typing.Iterable[str], ktfmt_style: typing.Optional[str], autofix: bool) -> int:
jar = _download_ktfmt_formatter_jar(ktfmt_version)
ktfmt_args = ["--set-exit-if-changed"]
if ktfmt_style is not None:
ktfmt_args.append(f"--{ktfmt_style}-style")
if not autofix:
ktfmt_args.append("--dry-run")
filenames = filenames if filenames else ["./"]
Copy link
Owner

Choose a reason for hiding this comment

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

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
>>> 

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

return_code, _, _ = run_command(
"java",
"-jar",
jar,
"ktfmt",
*ktfmt_args,
*filenames,
)
return return_code


def run_ktlint(ktlint_version: str, filenames:typing.Iterable[str], autofix: bool):

ktlint_jar = _download_ktlint_formatter_jar(ktlint_version)
jvm_args = ["--add-opens", "java.base/java.lang=ALL-UNNAMED"]

# ktlint does not return exit-code!=0 if we're formatting them.
Expand All @@ -79,14 +132,14 @@ def pretty_format_kotlin(argv: typing.Optional[typing.List[str]] = None) -> int:
"--reporter=json",
"--relative",
"--",
*_fix_paths(args.filenames),
*_fix_paths(filenames),
)

not_pretty_formatted_files: typing.Set[str] = set()
if return_code != 0:
not_pretty_formatted_files.update(item["file"] for item in json.loads(stdout))

if args.autofix:
if autofix:
print("Running ktlint format on {}".format(not_pretty_formatted_files))
run_command(
"java",
Expand All @@ -106,7 +159,7 @@ def pretty_format_kotlin(argv: typing.Optional[typing.List[str]] = None) -> int:
status = 1
print(
"{}: {}".format(
"The following files have been fixed by ktlint" if args.autofix else "The following files are not properly formatted",
"The following files have been fixed by ktlint" if autofix else "The following files are not properly formatted",
", ".join(sorted(not_pretty_formatted_files)),
),
)
Expand Down
2 changes: 1 addition & 1 deletion language_formatters_pre_commit_hooks/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
def run_command(*command: str) -> typing.Tuple[int, str, str]:
print(
"[cwd={cwd}] Run command: {command}".format(
command=command,
command=' '.join(command),
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I've rolled back.

cwd=os.getcwd(),
),
file=sys.stderr,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fun main(args: Array<String>) {
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding the end-to-end tests and examples.

I'm wondering if it would be better to have the ktfmt fixtures in a separate directories.
Possibly we could have the following fixtures paths

  • test-data/pretty_format_kotlin/ktlint <- move in there all the pre-PR fixture
  • test-data/pretty_format_kotlin/ktfmt <- add in here all the new fixtures related to ktfmt

Copy link
Author

Choose a reason for hiding this comment

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

Not too sure about this one, since we share some files between the two tests. Also I'd have to change some of the helper code to look in different directory based on the formatter.

I'm happy to do it if you really insist.

println("Hello, world!")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fun main(args: Array<String>) {
println("Hello, world!")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
fun main(args: Array<String>) {
println("Hello, world!")
}
5 changes: 3 additions & 2 deletions tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def run_autofix_test(
method: typing.Callable[[typing.List[str]], int],
not_pretty_formatted_path: str,
formatted_path: str,
extra_parameters: list[str],
Copy link
Owner

Choose a reason for hiding this comment

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

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 [])

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

) -> None:
tmpdir.mkdir("src")
not_pretty_formatted_tmp_path = tmpdir.join("src").join(basename(not_pretty_formatted_path))
Expand All @@ -55,14 +56,14 @@ def run_autofix_test(

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]
Copy link
Owner

Choose a reason for hiding this comment

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

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]

Copy link
Author

Choose a reason for hiding this comment

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

fixed

status_code = method(parameters)
if status_code != 1:
raise UnexpectedStatusCode(parameters=parameters, expected_status_code=1, actual_status_code=status_code)

# 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]
Copy link
Owner

Choose a reason for hiding this comment

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

as above

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

Copy link
Author

Choose a reason for hiding this comment

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

fixed

status_code = method(parameters)
if status_code != 0:
raise UnexpectedStatusCode(parameters=parameters, expected_status_code=0, actual_status_code=status_code)
Expand Down
71 changes: 66 additions & 5 deletions tests/pretty_format_kotlin_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
import pytest

from language_formatters_pre_commit_hooks import _get_default_version
from language_formatters_pre_commit_hooks.pretty_format_kotlin import _download_kotlin_formatter_jar
from language_formatters_pre_commit_hooks.pretty_format_kotlin import \
_download_ktlint_formatter_jar, _download_ktfmt_formatter_jar
Copy link
Owner

Choose a reason for hiding this comment

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

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

from language_formatters_pre_commit_hooks.pretty_format_kotlin import pretty_format_kotlin
from tests import change_dir_context
from tests import run_autofix_test
Expand Down Expand Up @@ -45,8 +46,21 @@ def undecorate_method():
)
@pytest.mark.integration
def test__download_kotlin_formatter_jar(ensure_download_possible, version): # noqa: F811
_download_kotlin_formatter_jar(version)
_download_ktlint_formatter_jar(version)

@pytest.mark.parametrize(
"version",
sorted(
{
_get_default_version("ktfmt"),
"0.45",
"0.46",
}
),
)
@pytest.mark.integration
def test__download_kotlin_formatter_jar(ensure_download_possible, version): # noqa: F811
_download_ktfmt_formatter_jar(version)

@pytest.mark.parametrize(
("filename", "expected_retval"),
Expand All @@ -57,9 +71,56 @@ def test__download_kotlin_formatter_jar(ensure_download_possible, version): # n
("NotPrettyFormattedFixed.kt", 0),
),
)
def test_pretty_format_kotlin(undecorate_method, filename, expected_retval):
def test_pretty_format_kotlin_ktlint(undecorate_method, filename, expected_retval):
assert undecorate_method([filename]) == expected_retval


def test_pretty_format_kotlin_autofix(tmpdir, undecorate_method):
run_autofix_test(tmpdir, undecorate_method, "NotPrettyFormatted.kt", "NotPrettyFormattedFixed.kt")
@pytest.mark.parametrize(
("filename", "expected_retval", "extra_parameters"),
(
("Invalid.kt", 1, []),
("PrettyFormatted.kt", 0, []),
("NotPrettyFormatted.kt", 1, []),
("NotPrettyFormattedFixedKtlint.kt", 0, []),
# ktfmt dropbox style (same as kotlinlang in this example)
("NotPrettyFormattedFixedKtfmtDropbox.kt", 1, ["--ktfmt"]),
("NotPrettyFormattedFixedKtfmtDropbox.kt", 0, ["--ktfmt", "--ktfmt-style=dropbox"]),
("NotPrettyFormattedFixedKtfmtDropbox.kt", 1, ["--ktfmt", "--ktfmt-style=google"]),
("NotPrettyFormattedFixedKtfmtDropbox.kt", 0, ["--ktfmt", "--ktfmt-style=kotlinlang"]),
# ktfmt google style
("NotPrettyFormattedFixedKtfmtGoogle.kt", 0, ["--ktfmt"]),
("NotPrettyFormattedFixedKtfmtGoogle.kt", 1, ["--ktfmt", "--ktfmt-style=dropbox"]),
("NotPrettyFormattedFixedKtfmtGoogle.kt", 0, ["--ktfmt", "--ktfmt-style=google"]),
("NotPrettyFormattedFixedKtfmtGoogle.kt", 1, ["--ktfmt", "--ktfmt-style=kotlinlang"]),
# ktfmt kotlinlang style (same as dropbox in this example)
("NotPrettyFormattedFixedKtfmtKotlinlang.kt", 1, ["--ktfmt"]),
("NotPrettyFormattedFixedKtfmtKotlinlang.kt", 0, ["--ktfmt", "--ktfmt-style=dropbox"]),
("NotPrettyFormattedFixedKtfmtKotlinlang.kt", 1, ["--ktfmt", "--ktfmt-style=google"]),
("NotPrettyFormattedFixedKtfmtKotlinlang.kt", 0, ["--ktfmt", "--ktfmt-style=kotlinlang"]),
# ktfmt other files
("NotPrettyFormatted.kt", 1, ["--ktfmt"]),
("Invalid.kt", 1, ["--ktfmt"]),
)
)
def test_pretty_format_kotlin(undecorate_method, filename, expected_retval, extra_parameters):
assert undecorate_method(extra_parameters + [filename]) == expected_retval
Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

Done, I agree this was a mouthful



@pytest.mark.parametrize(
("filename", "fixed_filename", "extra_parameters"),
(
("NotPrettyFormatted.kt", "NotPrettyFormattedFixedKtlint.kt", ["--autofix"]),
("NotPrettyFormatted.kt", "NotPrettyFormattedFixedKtfmtGoogle.kt", ["--autofix", "--ktfmt"]),
("NotPrettyFormatted.kt", "NotPrettyFormattedFixedKtfmtGoogle.kt", ["--autofix", "--ktfmt", "--ktfmt-style=google"]),
("NotPrettyFormatted.kt", "NotPrettyFormattedFixedKtfmtDropbox.kt", ["--autofix", "--ktfmt", "--ktfmt-style=dropbox"]),
("NotPrettyFormatted.kt", "NotPrettyFormattedFixedKtfmtKotlinlang.kt", ["--autofix", "--ktfmt", "--ktfmt-style=kotlinlang"]),
),
)
def test_pretty_format_kotlin_autofix(tmpdir, undecorate_method, filename, fixed_filename, extra_parameters):
run_autofix_test(
tmpdir,
undecorate_method,
filename,
fixed_filename,
extra_parameters
)
Loading