-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add support for ktfmt #196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
README.md
Outdated
@@ -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 |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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).
|
||
ktlint_jar = _download_kotlin_formatter_jar( | ||
args.ktlint_version, | ||
print(args) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_args.append(f"--{ktfmt_style}-style") | ||
if not autofix: | ||
ktfmt_args.append("--dry-run") | ||
filenames = filenames if filenames else ["./"] |
There was a problem hiding this comment.
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
>>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
@@ -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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
tests/__init__.py
Outdated
@@ -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], |
There was a problem hiding this comment.
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
extra_parameters: list[str], | |
extra_parameters: typing.Optional[typing.List[str]] = None, |
and while using extra_parameters
doing something like (extra_parameters or [])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
tests/__init__.py
Outdated
@@ -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] |
There was a problem hiding this comment.
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
parameters = extra_parameters + [not_pretty_formatted_tmp_strpath] | |
parameters = (extra_parameters or []) + ["--autofix", not_pretty_formatted_tmp_strpath] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tests/__init__.py
Outdated
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
parameters = extra_parameters + [not_pretty_formatted_tmp_strpath] | |
parameters = (extra_parameters or []) + ["--autofix", not_pretty_formatted_tmp_strpath] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tests/pretty_format_kotlin_test.py
Outdated
from language_formatters_pre_commit_hooks.pretty_format_kotlin import \ | ||
_download_ktlint_formatter_jar, _download_ktfmt_formatter_jar |
There was a problem hiding this comment.
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
tests/pretty_format_kotlin_test.py
Outdated
def test_pretty_format_kotlin(undecorate_method, filename, expected_retval, extra_parameters): | ||
assert undecorate_method(extra_parameters + [filename]) == expected_retval |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
Thanks for the feedback and spending the time to explain everything. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your changes.
Really appreciated the effort and the final results 😊
14cd9f4
to
9c2d33a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@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
Here's the help for
ktfmt
:Some notes:
--dry-run
mode which I use by default (and turn off with--autofix
--set-exit-if-changed
to be turned on so we get consistent error code (fail if a file is badly formatted)