reflex icon indicating copy to clipboard operation
reflex copied to clipboard

ruff-format: unify Black with Ruff `v0.1`

Open Borda opened this issue 5 months ago β€’ 18 comments

All Submissions:

  • [x] Have you followed the guidelines stated in CONTRIBUTING.md file?
  • [x] 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 avatar Mar 12 '24 00:03 Borda

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

Borda avatar Mar 12 '24 00:03 Borda

@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

picklelo avatar Mar 12 '24 01:03 picklelo

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 avatar Mar 12 '24 19:03 Borda

@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 avatar Mar 12 '24 20:03 Borda

@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.

picklelo avatar Mar 13 '24 19:03 picklelo

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 avatar Mar 13 '24 19:03 Borda

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

Borda avatar Mar 13 '24 20:03 Borda

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

masenf avatar Mar 13 '24 21:03 masenf

it seems this also would need to be updated: https://github.com/reflex-dev/reflex/blob/036afa951a5700b04722a0bcd7d3143dde07a12a/scripts/pyi_generator.py#L19-L20 Or is it fine to drop formatting for the pyi as they seem to be generated anyway?

Borda avatar Mar 13 '24 21:03 Borda

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

picklelo avatar Mar 13 '24 22:03 picklelo

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 avatar Mar 14 '24 14:03 Lendemor

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

Lendemor avatar Apr 22 '24 11:04 Lendemor

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

@Lendemor, merged with accepting all from main and re-run precommit... :flamingo:

Borda avatar Apr 22 '24 12:04 Borda

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

@Lendemor, merged with accepting all from main and re-run precommit... 🦩

I think you still need to remove some changes to pyi files, that's why the pre-commit check is failing.

Lendemor avatar Apr 22 '24 14:04 Lendemor

@picklelo @Lendemor mind check it now, pls :flamingo:

Borda avatar Apr 22 '24 18:04 Borda

@picklelo @Lendemor mind check it now, pls 🦩

I've reran the CI, it seems like some of the # type: ignore pragma have been moved by the formatting, so they are not on the line they should be, so pyright fail.

Lendemor avatar Apr 22 '24 21:04 Lendemor

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 πŸ‘

Lendemor avatar Apr 23 '24 13:04 Lendemor

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...

Borda avatar Apr 23 '24 13:04 Borda

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

Lendemor avatar Apr 23 '24 15:04 Lendemor

@Lendemor merged with main :flamingo:

Borda avatar Apr 23 '24 20:04 Borda