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

Use ruff instead of black/isort for autoformatting #3822

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Mar 28, 2024

Description

Part 1/2 for #3260

This only migrates the auto formatting capabilities. Linting will be in a separate PR which could use more scrutiny. Most of these changes look formatting changes and small things. See https://docs.astral.sh/ruff/formatter/black/ for known differences.

Type of change

Please delete options that are not relevant.

  • Chore

How Has This Been Tested?

N/A this only reformats the code

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR: I will send a PR after discussions are done :)

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@aabmass aabmass marked this pull request as ready for review March 28, 2024 20:17
@aabmass aabmass requested a review from a team as a code owner March 28, 2024 20:17
@aabmass aabmass force-pushed the ruff-fmt branch 3 times, most recently from 7a5d528 to 91e8019 Compare March 28, 2024 20:39
Copy link
Member Author

Choose a reason for hiding this comment

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

Actual changed file-please review

Copy link
Member Author

Choose a reason for hiding this comment

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

Actual changed file-please review

@xrmx
Copy link
Contributor

xrmx commented Mar 29, 2024

BTW do you see what's wrong with the Public API check?

@aabmass
Copy link
Member Author

aabmass commented Mar 29, 2024

BTW do you see what's wrong with the Public API check?

I think it's just flaking, the output doesn't have any results even though its failing

@aabmass aabmass requested a review from xrmx March 29, 2024 20:49
@aabmass
Copy link
Member Author

aabmass commented Mar 29, 2024

Actually @xrmx this PR fixes the check #3825

@xrmx
Copy link
Contributor

xrmx commented Mar 29, 2024

BTW do you see what's wrong with the Public API check?

I think it's just flaking, the output doesn't have any results even though its failing

I think at least something like this is required:

diff --git a/scripts/public_symbols_checker.py b/scripts/public_symbols_checker.py
index 05b7ad4a..b9802267 100644
--- a/scripts/public_symbols_checker.py
+++ b/scripts/public_symbols_checker.py
@@ -124,10 +124,12 @@ def remove_common_symbols():
 
 
 if added_symbols or removed_symbols:
-
     # If a symbol is added and removed in the same commit, we consider it
     # as not added or removed.
     remove_common_symbols()
+
+# re-evaluate symbols after remove_common_symbols side-effects
+if added_symbols or removed_symbols:
     print("The code in this branch adds the following public symbols:")
     print()
     for file_path_, symbols_ in added_symbols.items():

UPDATE: ah seen your PR :)

@xrmx
Copy link
Contributor

xrmx commented Mar 29, 2024

BTW changelog would be nice to have the ruff commit handy

pyproject.toml Show resolved Hide resolved
@aabmass
Copy link
Member Author

aabmass commented Apr 1, 2024

BTW changelog would be nice to have the ruff commit handy

I didn't add an entry initially because I don't think it's applicable to the changelog audience (consumers of the library). Happy to add if you think otherwise

@xrmx
Copy link
Contributor

xrmx commented Apr 2, 2024

BTW changelog would be nice to have the ruff commit handy

I didn't add an entry initially because I don't think it's applicable to the changelog audience (consumers of the library). Happy to add if you think otherwise

It would be handy for people poking with the code :)

@aabmass
Copy link
Member Author

aabmass commented Apr 2, 2024

Adding "do not merge label" until we decide for sure on Ruff replacing flake8 and pylint as well.

@ocelotl
Copy link
Contributor

ocelotl commented Apr 4, 2024

Adding "do not merge label" until we decide for sure on Ruff replacing flake8 and pylint as well.

Marking as draft to prevent accidental merging.

@ocelotl ocelotl marked this pull request as draft April 4, 2024 15:42
@xrmx
Copy link
Contributor

xrmx commented Apr 8, 2024

Once the conflict has been resolved this should be ready to land right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants