-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Ambari-26147: Add Ruff integration to ambari #3844
base: trunk
Are you sure you want to change the base?
Conversation
4202a64
to
bfc3374
Compare
abef2a0
to
7b0e3cb
Compare
@virajjasani Could you help check this PR? |
Wow, this is huge and yet it seems worth having. Skimmed through at high level, looks good. I wonder if anyone else is also interested to review this, otherwise let's get this in and see how things go. Nice work! |
Hi @JiaLiangC, |
@arshadmohammad Good catch! Your deployment failed because of this PR: #3770, which introduced numerous bugs in the frontend. |
@arshadmohammad Already resolved by this pr #3849. |
Thanks @JiaLiangC, I will review this patch |
@arshadmohammad I have added the usage of Ruff involved in this PR to the description. Additionally, all code changes in this PR are the result of ruff format. |
Hi @JiaLiangC , many changes include indentation / spacing between the lines, the current codebase uses two spaces for indentation / spacing instead of the typical four spaces / tabs. Is it possible to configure ruff to consider two spaces as the indentation and spacing size as it is the standard followed. Doing so may help reduce the size of the change as well and have clear diff for the actual changes. |
@vishalsuvagia Thank you for the reminder. I've changed the indent-width to 2 and committed it. This indeed makes it easier to review. However, there are still a massive 900 files, which I believe makes manual review very difficult. We can only rely on unit tests and subsequent bug reports after usage. Currently, the unit tests have passed, and I've deployed a complete cluster in my environment without finding any issues so far. Theoretically, code formatting tools like ruff shouldn't change the code semantics. |
What changes were proposed in this pull request?
The entire Ambari project has accumulated a large amount of Python code, which is the result of the collective efforts of multiple contributors to make Ambari better.
During this process, a problem has emerged: everyone has different coding styles, which leads to inconsistencies in the submitted code. This can result in various inconveniences, such as increased understanding costs of the underlying principles and greater difficulty in identifying the root causes of issues. As the codebase grows, these problems become increasingly apparent. Therefore, we are introducing ruff, along with some additional processes, to format the code and alleviate these issues.
What is Ruff?
An extremely fast Python linter and code formatter, written in Rust. Its speed is one of its advantages. More information can be found here: ruff.
(Please fill in changes proposed in this fix)
Ruff rules are derived from the Apache Superset project's rule file.
Ruff integration plan:
Notice: This PR's Ruff rules ignore some rule checks because these rules involve significant changes that will be addressed in subsequent PRs. The current PR ensures that all Python indentation and basic formatting conform to standards.
Ruff Usage Instructions in Ambari Code Submission
Install Ruff:
Format Code with Ruff:
ruff format .
Check Code Format Manually:
ruff check .
Check Code Format with Maven:
Fix Code Format with Maven:
How was this patch tested?
manual test CI/CD
After formatting thousands of files with Ruff, I deployed a simple cluster to verify that Ambari runs smoothly.
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)
Please review Ambari Contributing Guide before opening a pull request.