Document ruff and propose a transition plan #22
Conversation
Signed-off-by: Alina Buzachis <[email protected]>
There was a problem hiding this comment.
This is really helpful and I like the transition plan outlined here! A question: are we envisioning transitioning all our collections at once or starting with one?
And a comment: ruff is not a replacement for mypy and its documentation recommends using a type checker along with ruff. Since we haven't actually implemented mypy in most (or any?) of our repos, this isn't really a problem but I would update the transition plan to remove mypy from the linters we are replacing. We might also want to add an issue to the backlog for investigating type checkers since mypy is not the only option.
Signed-off-by: Alina Buzachis <[email protected]>
Yes, it isn't, thanks! That was a copy-paste oversight of the list of linters, fixed. I can do that! |
CI/linters.md
Outdated
| ``` | ||
|
|
||
| - Monitor `ruff`'s output and identify any discrepancies or issues compared to the existing linters. This phase should last for a trial period of 1 month. | ||
| - Validate `ruff`'s behavior and performance in the CI pipeline, ddress any discrepancies and fine-tune the `ruff` configuration as needed. |
There was a problem hiding this comment.
| - Validate `ruff`'s behavior and performance in the CI pipeline, ddress any discrepancies and fine-tune the `ruff` configuration as needed. | |
| - Validate `ruff`'s behavior and performance in the CI pipeline, address any discrepancies and fine-tune the `ruff` configuration as needed. |
CI/linters.md
Outdated
| docstring-code-line-length = "dynamic" | ||
| ``` | ||
|
|
||
| ### Integrating Ruff with Pre-Commit Hooks |
There was a problem hiding this comment.
I'm not sure about this one. I think pre-commit hooks should be used sparingly given that they get in the way of local development. We should support a development workflow that allows for WIP commits locally that don't pass linters. The enforcement of code quality standards happens in CI.
There was a problem hiding this comment.
There is also a git pre-push hook that we could consider as an alternative to pre-commit.
There was a problem hiding this comment.
Yeah, pre-push seems like a more appropriate place to do this. Though, I also just realized, you can't distribute git hooks with a repo, not ready to use anyways. How are we intending to use hooks? Is it just advisory?
There was a problem hiding this comment.
I think you can...I feel like a couple of the other repos I've worked on recently came with their own pre-commit config (and one used pre-push, which is how I learned about that). This suggests that it is possible: https://www.redhat.com/sysadmin/git-hooks
There was a problem hiding this comment.
Other teams use already precommit hooks https://github.com/ansible-collections/cisco.ios/blob/main/.pre-commit-config.yaml. However, I would remove this section from this file for the moment and I will explain better the approach in this PR #23.
Signed-off-by: Alina Buzachis <[email protected]>
There was a problem hiding this comment.
If the initial transition step involves testing Ruff configurations on all repositories within the cloud collection, we can use the configure_ci tool for this purpose.
repository-organization.md
Outdated
| @@ -0,0 +1,4 @@ | |||
| # Cloud Content Handbook Repository | |||
There was a problem hiding this comment.
Is there a reason for adding this file?
| - [flake8](https://flake8.pycqa.org/en/latest/) | ||
| - [isort](https://pycqa.github.io/isort/index.html) | ||
| - [mypy](https://mypy.readthedocs.io/en/stable/) | ||
| - runs the following linters (see [linters documentation](https://github.com/ansible-collections/cloud-content-handbook/blob/main/CI/linters.md)) to ensure the code complies with standard Ansible and python syntax/style/formatting conventions. |
There was a problem hiding this comment.
The linters.md file mentioned here actually contains the plan for transition. I'm uncertain if this file should be referenced here to list the linters we are presently utilizing.
There was a problem hiding this comment.
We do not really use mypy type checked in all the collections, but we mention it. I do not know though, do you have any suggestion?
There was a problem hiding this comment.
I only reference the file directly, to avoid content duplication, but I'm open to suggestions.
There was a problem hiding this comment.
I would prefer adding a separate section in https://github.com/ansible-collections/cloud-content-handbook/pull/22/files#diff-bfb9c5fe8e6c3b666231faaa4f979e64331975ef359bab303d71a6f56ff5b6ac to list the current linters and then linking to that section here. Additionally, I suggest renaming linters.md to a more meaningful name that indicates the file contains a transition plan and not just information about the linters.
There was a problem hiding this comment.
Tried to do that.
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
SUMMARY
Document ruff and propose a transition plan
Example PR ansible-collections/amazon.aws#2070
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION