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

ruff-format: unify Black with Ruff v0.1 #2837

Merged
merged 27 commits into from
Apr 23, 2024
Merged

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Mar 12, 2024

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

As Ruff lint is already used I would suggest using also ruff-format for better consistency between lint and formatted

fixes #2928

@Borda
Copy link
Contributor Author

Borda commented Mar 12, 2024

well while the Black was back from 2022, it seems the diff is quite large...

@picklelo
Copy link
Contributor

@Borda seems most of the large diff is adding a blank line after the final Returns: - is there a way to disable that? Then it may be much smaller

@Borda
Copy link
Contributor Author

Borda commented Mar 12, 2024

most of the large diff is adding a blank line after the final Returns: - is there a way to disable that? Then it may be much smaller

I found that these changes would be skipped case-by-case, but that would help with diff size at all :(
ref: https://docs.astral.sh/ruff/formatter/#format-suppression

@Borda
Copy link
Contributor Author

Borda commented Mar 12, 2024

@picklelo and the formating evolves over time, and the actual version of Ruff is before 0.1, so a fine middle ground would be just update to Ruff 0.1 with enabled formatting, and if you agree do smaller steps by updating to 0.2 and 0.3 in separate/consecutive/incremental PRs

@Borda Borda changed the title ruff-format: unify Black with Ruff ruff-format: unify Black with Ruff v0.1 Mar 12, 2024
Co-authored-by: Thomas Brandého <thomas.brandeho@gmail.com>
@picklelo
Copy link
Contributor

@Borda yes we should update to the latest version of ruff I didn't realize we were so out of date. Is there a huge change if we go to 0.3 of ruff now? If the diff is manageable I'm for upgrading to that. Otherwise, we can do the incremental approach as you suggested.

@Borda
Copy link
Contributor Author

Borda commented Mar 13, 2024

Is there a huge change if we go to 0.3 of ruff now?

I do not think in ruff lint but as you saw some patterns changed in Black/Ruff formating
if pre-commit as bot is enabled it has also a feature to update its version monthly via PR

@Borda
Copy link
Contributor Author

Borda commented Mar 13, 2024

looking to the pyright and not sure how to read the issues, what to fix...

@masenf
Copy link
Collaborator

masenf commented Mar 13, 2024

looking to the pyright and not sure how to read the issues, what to fix...

i would assume it's because the formatter moved important # type: ignore comments to the wrong place and thus they stopped applying to the line they were meant to apply to

@Borda
Copy link
Contributor Author

Borda commented Mar 13, 2024

it seems this also would need to be updated:

import black
import black.mode

Or is it fine to drop formatting for the pyi as they seem to be generated anyway?

@picklelo
Copy link
Contributor

Yeah less concerned about formatting the pyi files we can add that in later if needed.

@Lendemor
Copy link
Collaborator

We could drop the formatting of .pyi files but we have to make sure that the output of the script is deterministic, to avoid false positive when detecting is pyi were updated.

@Lendemor
Copy link
Collaborator

Can we sync this PR on main again since there is quite a few conflicts?

Co-authored-by: Thomas Brandého <thomas.brandeho@gmail.com>
Co-authored-by: Thomas Brandého <thomas.brandeho@gmail.com>
@Lendemor
Copy link
Collaborator

PR should be ready for merge, but it seems like github changed stuff about the available macos worker, we'll need to fix it first 👍

@Borda
Copy link
Contributor Author

Borda commented Apr 23, 2024

PR should be ready for merge

Thank you for your help and patience! 🙏

but it seems like github changed stuff about the available macos worker, we'll need to fix it first 👍

You just need to pin mac-13
Seems like GH swap mac14 from beta to latest...

@Lendemor
Copy link
Collaborator

We've merged a fix for macos CI, one last sync with main and we should be good to go. 🎉

@Borda Borda requested a review from Lendemor April 23, 2024 19:44
@Borda
Copy link
Contributor Author

Borda commented Apr 23, 2024

@Lendemor merged with main 🦩

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

Lets merge it before more conflicts arise!!! 😁

@masenf masenf merged commit 4d567b7 into reflex-dev:main Apr 23, 2024
46 checks passed
@Borda Borda deleted the ruff-format branch April 24, 2024 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fully Migrate off Black to Ruff
4 participants