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

[Fix] Swap package dependency ruff for black and autoflake #757

Merged
merged 13 commits into from
Sep 30, 2024

Conversation

maxschulz-COL
Copy link
Contributor

@maxschulz-COL maxschulz-COL commented Sep 30, 2024

Description

Given our continued difficulties to make ruff run on WebAssembly, we decide to swap the usage of ruff via subprocess for using black and autoflake. Using ruff is still the preferred goal, but as long as there is no python API, as well as a WASM compiled version available, we will stick with the current solution.

Note:
ruff was previous used in 2 places for Vizro-AI code string formatting:

inside vizro-ai plot
inside vizro-core pydantic to python (vizro models to python code) conversion.
This PR is mainly to drop the ruff use for these two.

Screenshot

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@github-actions github-actions bot added the Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package label Sep 30, 2024
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Is our goal to replace ruff completely or just as a mkdocs requirement?

For example there are still some mentions of ruff and we use it e.g. for pydocstyle etc.
Does this not have to be replaced? Or is this fine because it's run via our pre-commit hook?


[tool.ruff]
line-length = 120

[tool.ruff.format]
docstring-code-format = true

[tool.ruff.lint.pydocstyle]
convention = "google"

[tool.ruff.lint.pylint]
max-args = 6

...

@lingyielia
Copy link
Contributor

lingyielia commented Sep 30, 2024

Is our goal to replace ruff completely or just as a mkdocs requirement?

For example there are still some mentions of ruff and we use it e.g. for pydocstyle etc. Does this not have to be replaced? Or is this fine because it's run via our pre-commit hook?


[tool.ruff]
line-length = 120

[tool.ruff.format]
docstring-code-format = true

[tool.ruff.lint.pydocstyle]
convention = "google"

[tool.ruff.lint.pylint]
max-args = 6

...

ruff was previous used in 2 places for Vizro-AI code string formatting:

  1. inside vizro-ai plot
  2. inside vizro-core pydantic to python (vizro models to python code) conversion.

This PR is mainly to drop the ruff use for these two.

@huong-li-nguyen
Copy link
Contributor

ruff was previous used in 2 places for Vizro-AI code string formatting:

  1. inside vizro-ai plot
  2. inside vizro-core pydantic to python (vizro models to python code) conversion.

This PR is mainly to drop the ruff use for these two.

Got it! Can we add that to the PR description? :)

@huong-li-nguyen huong-li-nguyen self-requested a review September 30, 2024 15:36
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

A sad but necessary change :(

Please could you give a quick example of how this output is different to what ruff gave before? I guess the fact that none of the unit tests were changed means it's exactly the same for all our example cases?

vizro-ai/src/vizro_ai/plot/_response_models.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/models/_base.py Outdated Show resolved Hide resolved
@maxschulz-COL
Copy link
Contributor Author

A sad but necessary change :(

Please could you give a quick example of how this output is different to what ruff gave before? I guess the fact that none of the unit tests were changed means it's exactly the same for all our example cases?

Yes, the magic of unit tests :)

So in general not all code string would end up the same, as we are only formatting with black rules (and they may not 100% match - but seem to for the examples) and we only remove unused imports, and no other formats (which previously was the case for ruff but it looks like it was also unused)

@maxschulz-COL
Copy link
Contributor Author

Is our goal to replace ruff completely or just as a mkdocs requirement?

We are not removing our development dependency and tooling, but the package dependency.

@maxschulz-COL maxschulz-COL changed the title [Fix] Swap ruff for black and autoflake [Fix] Swap package dependency ruff for black and autoflake Sep 30, 2024
@maxschulz-COL maxschulz-COL merged commit 88bac23 into main Sep 30, 2024
39 checks passed
@maxschulz-COL maxschulz-COL deleted the tidy/swap_ruff_for_black branch September 30, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants