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

[CI] Format python with ruff #3832

Open
wants to merge 1 commit into
base: nightly
Choose a base branch
from
Open

Conversation

keith
Copy link
Contributor

@keith keith commented Dec 2, 2024

Ruff is a faster and more modern python formatter. This swaps out using
mojo format for python with ruff.

@keith keith requested review from jackos and a team as code owners December 2, 2024 18:05
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

The PR title does not conform to the '[<Project>] Title' format. Please update the PR title.

Typical [<Project>] values include:

  • [stdlib] — indicates a change to the Mojo standard library code
  • [docs] — indicates a change to the documentation

It's okay to include multiple labels on a PR that affect multiple areas of work.

Thank you for contributing to Mojo!🔥

You can also use a tool like www.regex101.com to see why your PR title fails to conform. Use ^(Revert ")?(\[\S.*\]\s?)+\s+[a-zA-Z`].* as the regex to test and Format python with ruff as the test string.

@keith keith force-pushed the ks/format-python-with-ruff branch from 0f3327f to e5cf021 Compare December 2, 2024 18:05
@keith keith changed the title Format python with ruff [CI] Format python with ruff Dec 2, 2024
@github-actions github-actions bot dismissed their stale review December 2, 2024 18:05

All good now, thanks!🫸🫷

@keith keith requested a review from JoeLoser December 2, 2024 18:06
@keith keith force-pushed the ks/format-python-with-ruff branch from e5cf021 to 3ba29f0 Compare December 2, 2024 18:08
Copy link
Contributor

@martinvuyk martinvuyk left a comment

Choose a reason for hiding this comment

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

@keith I think this will cause conflicts for every PR until mojo format behaves the same. I normally run mojo format ./stdlib/ for example. Are there plans to change mojo format to behave like ruff instead of black formatter?

stdlib/test/lit.cfg.py Outdated Show resolved Hide resolved
@keith keith force-pushed the ks/format-python-with-ruff branch from 3ba29f0 to fba3175 Compare December 2, 2024 18:29
Ruff is a faster and more modern python formatter. This swaps out using
`mojo format` for python with ruff.

Signed-off-by: Keith Smiley <[email protected]>
@keith keith force-pushed the ks/format-python-with-ruff branch from fba3175 to 0439b8e Compare December 2, 2024 18:40
@martinvuyk
Copy link
Contributor

@keith the tool having a static config option already sold me on wanting to use it 😄 (although now I read that black formatter also has that, I had no idea). Is there another option to avoid the difference in newline in PR #3822 (file) ?

@keith
Copy link
Contributor Author

keith commented Dec 2, 2024

I think we might just want to stop mojo format from running on python, but I'm hoping @JoeLoser can provide an opinion here

@martinvuyk
Copy link
Contributor

martinvuyk commented Dec 2, 2024

I think we might just want to stop mojo format from running on python, but I'm hoping @JoeLoser can provide an opinion here

Just an idea: add a flag that is default mojo only where you can also make mojo format run on any extension e.g. mojo format -e py -e ipynb. That way for people doing hybrid projects they can leverage mojo format directly, and those who don't want to aren't forced by it.

@walter-erquinigo
Copy link
Contributor

walter-erquinigo commented Dec 2, 2024

Hi! I'm the maintainer of mojo format and we never really intended to have mojo format be used on python files =P
So, the change that we'll do is to prevent it from touching any non-mojo files. Hopefully I'll land that this week.

Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

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

LGTM but you'll need to split this across two PRs since the .pre-commit-config.yaml isn't managed by Copybara whereas the rest of the files are I believe. Also, in a follow-up PR, can you add Ruff to the contributing docs for formatting Python-related parts to avoid future confusion?

@patrickdoc
Copy link
Contributor

!sync (for testing)

@patrickdoc patrickdoc removed their assignment Dec 5, 2024
@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Dec 5, 2024
@patrickdoc
Copy link
Contributor

!sync (testing 2)

@JoeLoser
Copy link
Collaborator

JoeLoser commented Dec 9, 2024

LGTM but you'll need to split this across two PRs since the .pre-commit-config.yaml isn't managed by Copybara whereas the rest of the files are I believe. Also, in a follow-up PR, can you add Ruff to the contributing docs for formatting Python-related parts to avoid future confusion?

Gentle ping here @keith as pretty much every PR from other contributors keeps modifying the my_module.py file since mojo format formats it and then it breaks the lint job internally requiring me to fix up each synced PR.

@walter-erquinigo
Copy link
Contributor

https://github.com/modularml/modular/pull/52586 should fix the mblack part

modularbot pushed a commit that referenced this pull request Dec 11, 2024
For context, in #3832 we decided
that mblack (aka mojo format) will no longer format non mojo files. The
change is trivial fortunately.
However, this doesn't solve the problem if formatting mojo cells in
notebooks, but that's a problem for the future.

MODULAR_ORIG_COMMIT_REV_ID: a28bc715affc0d83bff92b72c7eb313e1f80db96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants