Skip to content

Conversation

@ldkv
Copy link
Contributor

@ldkv ldkv commented Nov 28, 2025

No description provided.

@ldkv ldkv changed the title Minor improvements chore: replace black with ruff as formatter Nov 28, 2025
@ldkv ldkv force-pushed the minor-improvements branch from cb4f317 to 1dc89b2 Compare November 28, 2025 21:35
@c0rychu
Copy link
Owner

c0rychu commented Nov 28, 2025

@ldkv Sorry, can you rebase onto main again? I'm not sure why the GitHub diff did not show the diff to the new main.

@ldkv ldkv force-pushed the minor-improvements branch from 1dc89b2 to f651c7c Compare November 28, 2025 23:44
@ldkv
Copy link
Contributor Author

ldkv commented Nov 28, 2025

@c0rychu It is normal since there is no conflict with main. It is not necessary to rebase on main in that case.

@c0rychu c0rychu merged commit ec4deee into c0rychu:main Nov 28, 2025
10 checks passed
@c0rychu
Copy link
Owner

c0rychu commented Nov 29, 2025

@ldkv
Thanks for the cache thing, I'll keep that commit. But I'll revert the black one for now, since ruff can't automatically fix E501 Line too long. Appreciate your help, and if we find a way to make ruff do that, I'll be happy to adopt it again.

A test:

$ uv run ruff check --fix .
E501 Line too long (92 > 88)
  --> tests/test_path_utils.py:33:89
   |
31 | def test_create_windows_junction_invalid_target(tmp_path: Path):
32 |     with pytest.raises(ValueError):
33 |         create_windows_junction( symlink=tmp_path / "any", target=tmp_path / "nonexistent" )
   |                                                                                         ^^^^
   |

Found 1 error.

@ldkv
Copy link
Contributor Author

ldkv commented Nov 29, 2025

@ldkv Thanks for the cache thing, I'll keep that commit. But I'll revert the black one for now, since ruff can't automatically fix E501 Line too long. Appreciate your help, and if we find a way to make ruff do that, I'll be happy to adopt it again.

A test:

$ uv run ruff check --fix .
E501 Line too long (92 > 88)
  --> tests/test_path_utils.py:33:89
   |
31 | def test_create_windows_junction_invalid_target(tmp_path: Path):
32 |     with pytest.raises(ValueError):
33 |         create_windows_junction( symlink=tmp_path / "any", target=tmp_path / "nonexistent" )
   |                                                                                         ^^^^
   |

Found 1 error.

ruff check is for the linter. The command to format is:

uv run ruff format .

It should format your example automatically.

@ldkv ldkv deleted the minor-improvements branch November 29, 2025 15:14
@c0rychu
Copy link
Owner

c0rychu commented Nov 29, 2025

@ldkv Oh, you are right. I'm so dumb.
I think I got it right this time. in 5098c2f
The only difference is that I add --ignore E501 in ruff check --fix, but still keep it checks the E501 Line Too Long in the ruff check .

# Makefile
fix: ## Auto-fix lint/format issues via Ruff (will modify code!)
	uv run pyproject-fmt pyproject.toml
	uv run ruff check --fix --ignore E501 .
	uv run ruff format .

check-lint: ## Ruff lint (check only)
	uv run ruff check .

So, just delegate the fixing for E501 to ruff format

Hope that we can replace pyright by pyrefly once pyrefly is out of beta.

@ldkv
Copy link
Contributor Author

ldkv commented Nov 29, 2025

@c0rychu You should put the format before the check, no need to ignore E501.

@c0rychu
Copy link
Owner

c0rychu commented Nov 30, 2025

@ldkv
I'm just confused if I format first, is there any chance the check-- fix can introduce something that can be re-formatted?

In any case, really appreciate your help! As you can tell, I'm still learning how to get a good code quality check pipeline and CI on GitHub. This is something I'm not familiar with and get to learn as I work on this project.

Wish they can unifiy the linter and formatter in ruff, as mentioned in astral-sh/ruff#8232

@ldkv
Copy link
Contributor Author

ldkv commented Nov 30, 2025

There is a separate command for format check. Normally the format and linter fixes should be done before the PR by the devs.

The CI only includes the checks (format + linter + type checker) to enforce that the code follow your standard, the CI shouldn't fix the code itself.

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.

2 participants