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

Fixed VS Code format on save #1028

Merged
merged 2 commits into from
Feb 4, 2024

Conversation

rickardp
Copy link
Contributor

@rickardp rickardp commented Feb 4, 2024

There is a pipeline linter now that checks that the files are correctly formatted.

This PR will make VS Code format the files correctly on save.

The .editorconfig should work with most IDEs.

Copy link

github-actions bot commented Feb 4, 2024

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@rickardp
Copy link
Contributor Author

rickardp commented Feb 4, 2024

There is now two code formatters configured for this project: Yapf and Ruff. Would it make sense to align on one of them?

@akx
Copy link
Contributor

akx commented Feb 4, 2024

This repo isn't using ruff format (or black) for Python files; the Yapf configuration file was just copied over here but it too has never been actually run.
There was discussion about code formatting in another PR, but no concrete decision was made. (I'd prefer ruff format with a line-length of, say, 120.)

This change is fine, though, running the Ruff lint autofixes via VSCode and having a compliant editorconfig is a great DX improvement.

Copy link
Contributor

@akx akx left a comment

Choose a reason for hiding this comment

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

👍

@rickardp
Copy link
Contributor Author

rickardp commented Feb 4, 2024

This repo isn't using ruff format (or black) for Python files; the Yapf configuration file was just copied over here but it too has never been actually run. There was discussion about code formatting in another PR, but no concrete decision was made. (I'd prefer ruff format with a line-length of, say, 120.)

This change is fine, though, running the Ruff lint autofixes via VSCode and having a compliant editorconfig is a great DX improvement.

Yeah, I just learned of Ruff from your PR actually. I've always been using black, but Ruff is lightning fast and seems to share the same no-bikeshedding design goal, which I really like. I actually thought Ruff was running formatting via the pre-commit toolchain as it wanted to delete duplicate linefeeds etc, but I did not look into it further. I agree that Ruff seems like a good choice, and I do think that running a complete Python format-on-save would be a quality of life improvement

@akx
Copy link
Contributor

akx commented Feb 4, 2024

Yeah, Ruff has two personas, as it were: the linter, which has most of flake8's rules implemented, but notably not the whitespace rules just yet (there's a PR pending for that though: astral-sh/ruff#9266) and a Black-compatible formatter, which also cleans up whitespace to adhere to the same rules, but there's no way to apply just that part of the formatter (for technical raisins). There's a clear usecase for only wanting (part of) the whitespace rules instead of the wholesale reformatting that the formatter would apply too :)

@Titus-von-Koeller Titus-von-Koeller merged commit ecf51cb into bitsandbytes-foundation:main Feb 4, 2024
6 checks passed
@Titus-von-Koeller
Copy link
Collaborator

Hey @rickardp,

thanks for the PR. This useful.

@Titus-von-Koeller
Copy link
Collaborator

@akx

Actually, I had posted an update with a decision to go forward with the black / ruff format approach and deprecate yapf.

See #994 (comment)

We should also set up a c-format formatter for C/C++/CUDA.

Thanks a lot both! This kind of work will make it much easier for everyone to collaborate and contribute.

I ll be back from vacation in a week.

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.

3 participants