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

feat: add black to pyproject.toml #3559

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jan 29, 2024

Based off upstream addition in psf/black#4181 (comment) (plus a small bugfix I'll also push there).

Note that this is idealized, that is, if a type is a bool, it expects a bool. Due to the way this was implemented in black, strings are supported everywhere, ints also can be used for bools, and _ is equivalent to -. Tightening this is proposed in psf/black#4178, and of the 23,128 pyproject.toml's on PyPI that contain tool.black, only about 2% use weird types and about 5% use _ instead of -. Since these are just allowed by implementation details, and are never shown in docs, etc, I think it's fine to leave this simple and only support the "correct" way to do things? If not, I can add this (just for the SchemaStore version). At least for reasonable/common versions. Technically, a mix like this is valid:

skip-magic_trailing-comma = "0"

But upstream doesn't support it in the embedded schema and I'd rather not here as well if that's okay.

Happy to rebase after #3556 if needed.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@github-actions github-actions bot added schema-validation.json gruntfile gruntfile.js is updated (auto-generated by labeler action) labels Jan 29, 2024
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @madskristensen and @hyperupcall - if they write a comment saying "LGTM" then it will be merged.

@madskristensen
Copy link
Contributor

@hyperupcall thoughts?

@JelleZijlstra
Copy link

Thanks for doing this!

Note that this is idealized, that is, if a type is a bool, it expects a bool. Due to the way this was implemented in black, strings are supported everywhere, ints also can be used for bools, and _ is equivalent to -.

As a maintainer of Black I'd like to endorse this decision. We'll probably deprecate these nonstandard ways to configure the project in our documentation and possibly also at runtime. If anyone complains to SchemaStore about it, feel free to send them to our repo instead.

@hyperupcall
Copy link
Collaborator

hyperupcall commented Jan 30, 2024

Since these are just allowed by implementation details, and are never shown in docs, etc, I think it's fine to leave this simple and only support the "correct" way to do things?

In general, I support fixing schemas to not exclude shapes or values that are implementation details. But since usage is of either is relatively rare and it will be deprecated in feature versions of black, then I don't think it's necessary to include support for it in the schema upfront. If someone complains, I wouldn't be opposed to a MR to fix it, if it doesn't complicate the schema too much.

cc @madskristensen

If anyone complains to SchemaStore about it, feel free to send them to our repo instead.

Noted 👍

Copy link
Collaborator

@hyperupcall hyperupcall left a comment

Choose a reason for hiding this comment

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

LGTM!

@henryiii Also for the future, I would recommend putting "partials" in the missingCatalogUrl property of schema-validation.json (not in catalog.json). Don't worry about it in this PR though, I'm making fixes for it in all the places via PR #3556 (see commit 4b4a4b4 (#3556) for details). I wish I saw the use of that property sooner.

@henryiii
Copy link
Contributor Author

henryiii commented Jan 31, 2024

FYI, here's the top 20 most popular tool sections based on all pyproject.toml's on PyPI, with my personal notes on how hard it might be to add the missing ones:

  • tool.poetry: 45523
  • tool.black: 23128
  • tool.isort: 16874 (big dataclass, might be easy to transform)
  • tool.pytest: 15144 (not sealed, though always in a subtable tool.pytest.ini_options)
  • tool.setuptools: 11136
  • tool.mypy: 10279
  • tool.coverage: 8907 (doesn’t look too hard)
  • tool.ruff: 8485
  • tool.setuptools_scm: 6034 (easy)
  • tool.hatch: 4846
  • tool.pylint: 3725 (auto unnesting)
  • tool.pyright: 2948
  • tool.flit: 2826 (moderate)
  • tool.flake8: 1833 (unofficial)
  • tool.pdm: 1458
  • tool.tox: 1423 (one item!)
  • tool.cibuildwheel: 1303
  • tool.semantic_release: 1276
  • tool.poetry-dynamic-versioning: 982
  • tool.maturin: 848 (Maybe easy like Ruff?)

I'll probably do tox next, since it's literally one item (everything goes in a string in legacy_tox_ini), and people still manage to get it wrong:

tool.tox.*:
legacy_tox_ini: 1405
tox: 5
pytest: 1
flake8: 1
black: 1
pydocstyle: 1
mypy: 1
types-requests: 1
isort: 1
docopt: 1
envlist: 1
skipsdist: 1
name: 1
authors: 1
coverage: 1

How someone puts tool.tox.tox, I'm not sure, but five projects did. :)

tool.tox.tox = ...
gluestick 2.1.7 = '^3.24.4'
client-bank-1c 0.3 = {'envlist': 'py36', 'skipsdist': True}
odoo-commands 0.1.1.dev0 = {'envlist': 'py36, docs', 'skipsdist': True}
playwright-odoo 0.1.dev0 = {'envlist': 'py36', 'skipsdist': True}
odoo-stubs 0.1.dev0 = {'envlist': 'py38', 'skipsdist': True}

@hyperupcall hyperupcall mentioned this pull request Feb 1, 2024
20 tasks
@hyperupcall
Copy link
Collaborator

That list is very useful - I have put that into an issue #3564

@hyperupcall hyperupcall merged commit a1e0cbc into SchemaStore:master Feb 1, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gruntfile gruntfile.js is updated (auto-generated by labeler action) schema-validation.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants