pre-commit: SwiftLint: Disable failing tests #781
Conversation
|
I'm not sure I understand these changes in combination with the description you gave:
I do not see anything being disabled here. |
cclauss
changed the title
|
Yes. Moving to |
cclauss
changed the title
|
GitHub Actions can run pre-commit in 25 seconds if we remove SwiftLint. SwiftLint --fix can run in 25 seconds on the Future pull requests can remove more items from |
|
Please let me know what changes should be made. |
There was a problem hiding this comment.
LGTM now. Since I don't know much about the pre-commit configs, I'll let @JeneaVranceanu give the final approval.
|
On |
| - run: pip install pre-commit | ||
| - run: pre-commit --version | ||
| - run: pre-commit install | ||
| - run: pre-commit run --all-files |
There was a problem hiding this comment.
@cclauss I am a little bit confused.
All this is good but now we'll have codespell running in macOS-tests.yml and in pre-commit.yml if we open a PR against develop branch. And when swiftlint will be enabled in pre-commit-config.yaml (and I think it should be enabled now with the update you made to disabled_rules) we will have both swiftlint and codespell running twice.
There was a problem hiding this comment.
What was again the purpose of this workflow?
There was a problem hiding this comment.
Enable SwiftLint in GitHub Actions but disable the failing tests that cannot be auto-fixed so that we do not backslide on the tests that currently pass.
I see only one really useful use case for the |
| - block_based_kvo | ||
| - closure_body_length | ||
| - computed_accessors_order | ||
| - cyclomatic_complexity | ||
| - duplicate_imports | ||
| - empty_enum_arguments | ||
| - empty_string | ||
| - file_length | ||
| - for_where | ||
| - force_cast | ||
| - force_try | ||
| - force_unwrapping | ||
| - function_body_length | ||
| - function_parameter_count | ||
| - identifier_name | ||
| - implicit_getter | ||
| - implicitly_unwrapped_optional | ||
| - indentation_width | ||
| - large_tuple | ||
| - legacy_objc_type | ||
| - line_length | ||
| - multiple_closures_with_trailing_closure | ||
| - nesting | ||
| - orphaned_doc_comment | ||
| - operator_whitespace | ||
| - return_arrow_whitespace | ||
| - shorthand_operator | ||
| - todo | ||
| - trailing_closure | ||
| - type_body_length | ||
| - type_name | ||
| - unneeded_break_in_switch | ||
| - unused_optional_binding | ||
| - vertical_parameter_alignment | ||
| - xctfail_message |
There was a problem hiding this comment.
Thanks for the effort!
I think it will be easier to fix issues one by one.
There was a problem hiding this comment.
I'll resume fixes as soon as this PR is merged but for now, there are a few questions.
|
Correct that pre-commit.ci’s key differentiator is that it can commit auto-fixes so that humans don’t have to. |
|
What else is required on this pr? |
Reviewing now. Will let you know soon. |
|
@yaroslavyaroslav Please, be informed about the pros of using pre-commit CI: #781 (comment) |
|
@cclauss Done with the review. Looks good. |
|
Nice. It will be good to have guardrails in place on those SwiftLint tests that already pass. |
Summary of Changes
Fixes # (if applicable - add the number of issue this PR addresses)
Enable SwiftLint in GitHub Actions but disable the failing tests that cannot be auto-fixed so that we do not backslide on the tests that currently pass.
Test Data or Screenshots
By submitting this pull request, you are confirming the following:
developbranch.