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

Add taplo pre-commit hook to format and sort toml files #6781

Merged
merged 10 commits into from
Nov 12, 2024

Conversation

user27182
Copy link
Contributor

@user27182 user27182 commented Nov 11, 2024

Overview

toml files are not currently formatted or sorted. This enables things like:

pyvista/pyproject.toml

Lines 69 to 74 in a71e027

typing= [
'pyvista[pinned]',
'numpy>=2.0.0',
'mypy<1.14.0',
'npt-promote==0.1',
]

Where spaces around = are not enforced and where alphabetical sorting is not enforced (we have n->m->n order in the example)

This PR uses https://github.com/pappasam/toml-sort https://github.com/tamasfe/taplo to fix and enforce the formatting and sorting.

@pyvista-bot pyvista-bot added maintenance Low-impact maintenance activity dependencies Pull requests that update a dependency file labels Nov 11, 2024
@user27182 user27182 marked this pull request as draft November 11, 2024 18:05
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.49%. Comparing base (654782a) to head (d0eb514).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6781   +/-   ##
=======================================
  Coverage   97.49%   97.49%           
=======================================
  Files         143      143           
  Lines       28177    28177           
=======================================
  Hits        27472    27472           
  Misses        705      705           

@user27182 user27182 marked this pull request as ready for review November 11, 2024 19:23
pyproject.toml Outdated
in_place = true
# Sorting options:
no_sort_tables = true
sort_inline_arrays = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's desirable to have separate PRs, one for formatting and another for sorting, we can remove sort_inline_arrays = true and add this line in a separate PR

@pyvista-bot
Copy link
Contributor

pyvista-bot commented Nov 11, 2024

@pyvista-bot pyvista-bot temporarily deployed to pull request November 11, 2024 21:40 Inactive
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

+100 sorting TOML files. However, could you consider whether using Taplo might be a better option compared to toml-sort?

@user27182
Copy link
Contributor Author

+100 sorting TOML files. However, could you consider whether using Taplo might be a better option compared to toml-sort?

I have no preference, taplo seems to have similar options https://taplo.tamasfe.dev/configuration/formatter-options.html, so we could probably use it instead

hooks:
- id: taplo-format
# See options: https://taplo.tamasfe.dev/configuration/formatter-options.html
args: [--option, "reorder_arrays=true"]
Copy link
Contributor Author

@user27182 user27182 Nov 12, 2024

Choose a reason for hiding this comment

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

With fb0df95 I opted not to use indent_string, so the default indent of 2 spaces is used. This is also the default for toml-sort. I think it's okay to use the default 2 spaces, but we can use 4 if preferred

Suggested change
args: [--option, "reorder_arrays=true"]
args: [--option, "reorder_arrays=true", --option, "indent_string= "]

Have a look at the other options, are there any else we should include (can also add options in a separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff is better with "indent_string= " enabled if that's desired (i.e. fewer formatting changes). It's easier to see the sorting that way.

Copy link
Member

Choose a reason for hiding this comment

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

+1 using default.

pyproject.toml Outdated Show resolved Hide resolved
@user27182 user27182 changed the title Add toml-sort pre-commit hook and use it to format pyproject.toml Add taplo pre-commit hook to format and sort toml files Nov 12, 2024
@pyvista-bot pyvista-bot temporarily deployed to pull request November 12, 2024 02:00 Inactive
@tkoyama010
Copy link
Member

pre-commit.ci autofix

tkoyama010
tkoyama010 previously approved these changes Nov 12, 2024
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

LGTM

@pyvista-bot pyvista-bot temporarily deployed to pull request November 12, 2024 05:26 Inactive
@tkoyama010 tkoyama010 enabled auto-merge (squash) November 12, 2024 20:17
pyproject.toml Outdated
Comment on lines 63 to 67
'cmocean<4.0.4',
'colorcet<3.2.0',
'pyvista[pinned]',
# Embreex does not work with arm-based macs
'embreex<2.17.8; sys_platform != "darwin" or platform_machine != "arm64"',
Copy link
Contributor Author

@user27182 user27182 Nov 12, 2024

Choose a reason for hiding this comment

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

Is this a bug with the sorting? We have p before e, not a huge deal but not expected either.

Edit: looks like the comment between items may have thrown it off

Copy link
Member

Choose a reason for hiding this comment

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

Following #6782, let's move comment inline. Additionally, it may be helpful to consider submitting an issue to Taplo regarding why the sorting functionality is not working as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following #6782, let's move comment inline.

Fixed in d0eb514

Additionally, it may be helpful to consider submitting an issue to Taplo regarding why the sorting functionality is not working as expected.

Apparently having full-line comments split up the sorting is the expected behaviour, see tamasfe/taplo#675

So we should generally avoid using any full-line comments in any toml files with this tool.

@tkoyama010 tkoyama010 disabled auto-merge November 12, 2024 20:30
pyproject.toml Outdated
Comment on lines 63 to 67
'cmocean<4.0.4',
'colorcet<3.2.0',
'pyvista[pinned]',
# Embreex does not work with arm-based macs
'embreex<2.17.8; sys_platform != "darwin" or platform_machine != "arm64"',
Copy link
Member

Choose a reason for hiding this comment

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

Following #6782, let's move comment inline. Additionally, it may be helpful to consider submitting an issue to Taplo regarding why the sorting functionality is not working as expected.

@pyvista-bot pyvista-bot temporarily deployed to pull request November 12, 2024 20:56 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants