Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions common/src/table.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::cell::{RefCell, RefMut};

Check warning on line 1 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-fmt

Diff in /home/runner/work/toml-fmt/toml-fmt/common/src/table.rs
use std::collections::HashMap;
use std::collections::{HashMap, BTreeSet};
use std::iter::zip;
use std::ops::Index;

use taplo::syntax::SyntaxKind::{ENTRY, IDENT, KEY, NEWLINE, TABLE_ARRAY_HEADER, TABLE_HEADER, VALUE};
use taplo::syntax::{SyntaxElement, SyntaxNode};
use taplo::HashSet;
use taplo::{dom::node::DomNode as _, parser::parse};

use crate::create::{make_empty_newline, make_key, make_newline, make_table_entry};
use crate::string::load_text;
Expand Down Expand Up @@ -279,7 +280,7 @@
None
}

pub fn collapse_sub_tables(tables: &mut Tables, name: &str) {
pub fn collapse_sub_tables(tables: &mut Tables, name: &str, exclude: &[Vec<String>]) {
let h2p = tables.header_to_pos.clone();
let sub_name_prefix = format!("{name}.");
let sub_table_keys: Vec<&String> = h2p.keys().filter(|s| s.starts_with(sub_name_prefix.as_str())).collect();
Expand All @@ -296,6 +297,14 @@
if main_positions.len() != 1 {
return;
}

Check warning on line 300 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-fmt

Diff in /home/runner/work/toml-fmt/toml-fmt/common/src/table.rs
// remove `name` from `exclude`s (and skip if `name` is not a prefix)
let prefix = parse_ident(name).expect("could not parse prefix");
let exclude: BTreeSet<_> = exclude.into_iter().filter_map(|id| {

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-build

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-lint

this `.into_iter()` call is equivalent to `.iter()` and will not consume the `slice`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-lint

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-test

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-build

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-lint

this `.into_iter()` call is equivalent to `.iter()` and will not consume the `slice`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-lint

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-test

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-test

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-build

unused variable: `exclude`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-lint

this `.into_iter()` call is equivalent to `.iter()` and will not consume the `slice`

Check failure on line 303 in common/src/table.rs

View workflow job for this annotation

GitHub Actions / rust-lint

unused variable: `exclude`
let (prefix_2, rest) = id.split_at(prefix.len()+1);
(prefix == prefix_2).then_some(rest)
}).collect();

let mut main = tables.table_set[*main_positions.first().unwrap()].borrow_mut();
for key in sub_table_keys {
let sub_positions = tables.header_to_pos[key].clone();
Expand Down Expand Up @@ -340,3 +349,35 @@
sub.clear();
}
}

pub fn parse_ident(ident: &str) -> Result<Vec<String>, String> {
let parsed = parse(&format!("{ident} = 1"));
if let Some(e) = parsed.errors.first() {
return Err(format!("syntax error: {e}"));
}

let root = parsed.into_dom();
let errors = root.errors();
if let Some(e) = errors.get().first() {
return Err(format!("semantic error: {e}"));
}

dbg!(&root.errors());

// We cannot use `.into_syntax()` since only the DOM transformation
// allows accessing ident `.value()`s without quotes.
let mut node = root;
let mut parts = vec![];
while let Ok(table) = node.try_into_table() {
let entries = table.entries().get();
if entries.len() != 1 {
return Err("expected exactly one entry".to_string());
}
let mut it = entries.iter();
let (key, next_node) = it.next().unwrap(); // checked if len == 1 above

parts.push(key.value().to_string());
node = next_node.clone();
}
Ok(parts)
}
4 changes: 2 additions & 2 deletions pyproject-fmt/rust/src/dependency_groups.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use common::util::iter;
use lexical_sort::natural_lexical_cmp;
use std::cmp::Ordering;

pub fn fix(tables: &mut Tables, keep_full_version: bool) {
collapse_sub_tables(tables, "dependency-groups");
pub fn fix(tables: &mut Tables, keep_full_version: bool, do_not_collapse: &[Vec<String>]) {
collapse_sub_tables(tables, "dependency-groups", do_not_collapse);
let table_element = tables.get("dependency-groups");
if table_element.is_none() {
return;
Expand Down
23 changes: 20 additions & 3 deletions pyproject-fmt/rust/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use std::string::String;

use common::taplo::formatter::{format_syntax, Options};
use common::taplo::parser::parse;
use pyo3::exceptions::PyValueError;
use pyo3::prelude::{PyModule, PyModuleMethods};
use pyo3::types::PyTuple;
use pyo3::{pyclass, pyfunction, pymethods, pymodule, wrap_pyfunction, Bound, PyResult};

use crate::global::reorder_tables;
Expand All @@ -25,19 +27,21 @@ pub struct Settings {
max_supported_python: (u8, u8),
min_supported_python: (u8, u8),
generate_python_version_classifiers: bool,
do_not_collapse: Vec<Vec<String>>,
}

#[pymethods]
impl Settings {
#[new]
#[pyo3(signature = (*, column_width, indent, keep_full_version, max_supported_python, min_supported_python, generate_python_version_classifiers ))]
#[pyo3(signature = (*, column_width, indent, keep_full_version, max_supported_python, min_supported_python, generate_python_version_classifiers, do_not_collapse))]
const fn new(
column_width: usize,
indent: usize,
keep_full_version: bool,
max_supported_python: (u8, u8),
min_supported_python: (u8, u8),
generate_python_version_classifiers: bool,
do_not_collapse: Vec<Vec<String>>,
) -> Self {
Self {
column_width,
Expand All @@ -46,6 +50,7 @@ impl Settings {
max_supported_python,
min_supported_python,
generate_python_version_classifiers,
do_not_collapse,
}
}
}
Expand All @@ -64,9 +69,10 @@ pub fn format_toml(content: &str, opt: &Settings) -> String {
opt.max_supported_python,
opt.min_supported_python,
opt.generate_python_version_classifiers,
opt.do_not_collapse.as_slice(),
);
dependency_groups::fix(&mut tables, opt.keep_full_version);
ruff::fix(&mut tables);
dependency_groups::fix(&mut tables, opt.keep_full_version, opt.do_not_collapse.as_slice());
ruff::fix(&mut tables, opt.do_not_collapse.as_slice());
reorder_tables(&root_ast, &tables);

let options = Options {
Expand Down Expand Up @@ -94,13 +100,24 @@ pub fn format_toml(content: &str, opt: &Settings) -> String {
format_syntax(root_ast, options)
}

/// Parse a nested toml identifier into a tuple of idents
///
/// >>> parse_ident('a."b.c"')
/// ('a', 'b.c')
#[pyfunction]
pub fn parse_ident<'py>(py: pyo3::Python<'py>, ident: &str) -> PyResult<Bound<'py, PyTuple>> {
let parts = common::table::parse_ident(ident).map_err(|e| PyValueError::new_err(e))?;
PyTuple::new(py, parts)
}

/// # Errors
///
/// Will return `PyErr` if an error is raised during formatting.
#[pymodule]
#[pyo3(name = "_lib")]
pub fn _lib(m: &Bound<'_, PyModule>) -> PyResult<()> {
m.add_function(wrap_pyfunction!(format_toml, m)?)?;
m.add_function(wrap_pyfunction!(parse_ident, m)?)?;
m.add_class::<Settings>()?;
Ok(())
}
3 changes: 2 additions & 1 deletion pyproject-fmt/rust/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ pub fn fix(
max_supported_python: (u8, u8),
min_supported_python: (u8, u8),
generate_python_version_classifiers: bool,
do_not_collapse: &[Vec<String>],
) {
collapse_sub_tables(tables, "project");
collapse_sub_tables(tables, "project", do_not_collapse);
let table_element = tables.get("project");
if table_element.is_none() {
return;
Expand Down
4 changes: 2 additions & 2 deletions pyproject-fmt/rust/src/ruff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use common::table::{collapse_sub_tables, for_entries, reorder_table_keys, Tables
use lexical_sort::natural_lexical_cmp;

#[allow(clippy::too_many_lines)]
pub fn fix(tables: &mut Tables) {
collapse_sub_tables(tables, "tool.ruff");
pub fn fix(tables: &mut Tables, do_not_collapse: &[Vec<String>]) {
collapse_sub_tables(tables, "tool.ruff", do_not_collapse);
let table_element = tables.get("tool.ruff");
if table_element.is_none() {
return;
Expand Down
3 changes: 3 additions & 0 deletions pyproject-fmt/rust/src/tests/main_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ fn test_format_toml(
max_supported_python,
min_supported_python: (3, 9),
generate_python_version_classifiers: true,
do_not_collapse: vec![],
};
let got = format_toml(start, &settings);
assert_eq!(got, expected);
Expand All @@ -216,6 +217,7 @@ fn test_issue_24(data: PathBuf) {
max_supported_python: (3, 9),
min_supported_python: (3, 9),
generate_python_version_classifiers: true,
do_not_collapse: vec![],
};
let got = format_toml(start.as_str(), &settings);
let expected = read_to_string(data.join("ruff-order.expected.toml")).unwrap();
Expand Down Expand Up @@ -246,6 +248,7 @@ fn test_column_width() {
max_supported_python: (3, 13),
min_supported_python: (3, 13),
generate_python_version_classifiers: true,
do_not_collapse: vec![],
};
let got = format_toml(start, &settings);
let expected = indoc! {r#"
Expand Down
13 changes: 12 additions & 1 deletion pyproject-fmt/src/pyproject_fmt/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from toml_fmt_common import ArgumentGroup, FmtNamespace, TOMLFormatter, _build_cli, run # noqa: PLC2701

from pyproject_fmt._lib import Settings, format_toml
from pyproject_fmt._lib import Settings, format_toml, parse_ident

if TYPE_CHECKING:
from collections.abc import Sequence
Expand All @@ -19,6 +19,7 @@ class PyProjectFmtNamespace(FmtNamespace):
keep_full_version: bool
max_supported_python: tuple[int, int]
generate_python_version_classifiers: bool
do_not_collapse: list[tuple[str, ...]]


class PyProjectFormatter(TOMLFormatter[PyProjectFmtNamespace]):
Expand Down Expand Up @@ -73,6 +74,15 @@ def _version_argument(got: str) -> tuple[int, int]:
help="latest Python version the project supports (e.g. 3.14)",
)

parser.add_argument(
"--do-not-collapse",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I am on board with selective collapse I think I said in the issue too I would really prefer this to be on or off like rather than selective...

Copy link
Author

@flying-sheep flying-sheep Oct 15, 2025

Choose a reason for hiding this comment

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

Then I didn’t interpret the line

I was thinking something along the line of --tables flag with short or long values.

as you meant it. Can you please describe what you meant by that?

what’s a “short or long value”?

Copy link
Member

Choose a reason for hiding this comment

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

I do not want configurable collapse. It should be either collapse all or expand all 😊

Copy link
Author

Choose a reason for hiding this comment

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

I don’t understand what you mean! Expand/collapse can happen on multiple levels and neither “collapsing all” nor “expanding all” makes sense, as nobody would want that.

Currently, pyproject-fmt has an algorithm determining which collapsed table headers exist:

  • [project]
  • [build-system]
  • [tool.*]

Everything below these gets expanded.

“expanding all” means to me that there are no table headers, and project.name = "..." exists together with tool.ruff.lint.… = .... Nobody wants that.

“collapsing all” could mean that only table headers can contain dots, like the many many [ruff.lint.…] headers here:

[tool.ruff.lint.flake8-annotations]
suppress-none-returning = true
[tool.ruff.lint.flake8-bandit]
hardcoded-tmp-directory = ['Bar', 'ALPHA']
[tool.ruff.lint.flake8-boolean-trap]
extend-allowed-calls = ['Bar', 'ALPHA']
[tool.ruff.lint.flake8-bugbear]
extend-immutable-calls = ['Bar', 'ALPHA']
[tool.ruff.lint.flake8-builtins]
builtins-ignorelist = ['Bar', 'ALPHA']

Nobody wants that either.

Everyone using pyproject-fmt likes expanding things, except for a handful of super long collapsed tables, like described in #5 and #35

Copy link
Member

Choose a reason for hiding this comment

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

Expand/collapse can happen on multiple levels and neither “collapsing all” nor “expanding all” makes sense, as nobody would want that.

Ignore level 1. The project today collapses all supported table configurations of level 2+ into level 2. This is what I call collapse all.

Expand all would be to expand all configurations to be tables.

Everyone using pyproject-fmt likes expanding things, except for a handful of super long collapsed tables, like described in #5 and #35

Interesting idea, though I'm not sure how you surveyed everyone. I think this is a personal stylistic choice, and this is an opinionated formatter after all.

Copy link
Author

Choose a reason for hiding this comment

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

tool.ruff.lint.flake8-tidy-imports.banned-api."some.import".msg is just ridiculous, but that doesn’t mean I want to see dozens of table headers with a single entry each.

Can we find a rule that respects that?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a personal opinion but I don't find it that bad. 🤔

That being said I'm not totally opposed to this idea, but I'm also not invested in making it happen, as I find the status quo alright. If someone puts together a well tested PR I'll review, but I'll not invest time in trying to make it happen.

PS. Your tone is a bit aggressive here, I'd recommend stop throwing around harsh words like everyone, nobody or ridiculous; and choose some that respect the fact that we all have different tastes and opinions, and that's alright.

Copy link
Author

@flying-sheep flying-sheep Oct 15, 2025

Choose a reason for hiding this comment

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

I’m German, I tend to be a bit more direct than what Americans are accustomed to, and quite thankful when it’s pointed out that something could be perceived as harsh.

However, that requires the other side to assume the same humility. My tone isn’t aggressive, you perceive it as such. I’m not thankful for tone-policing that portrays such subjective matters as a fact.

I'm not totally opposed to this idea, but I'm also not invested in making it happen

Yeah, I think we’d need to come up with a rule/preset that makes sense. I totally get it that you don’t want total customizability.

To elaborate on my motivation for the current approach: I was thinking that the 1–2 levels you’re referring to above are not a numerical cutoff but semantic: we know there’s only build-system (1 level), project (1 level) and tool.* (2 levels). But we don’t know every single tool’s layering. That’s why I thought that hard-coding would be too much maintenance effort, and I can think of no heuristic that makes sense, and therefore opted for customizability.

So how would a rule look like that you could get behind?

Copy link
Member

Choose a reason for hiding this comment

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

However, that requires the other side to assume the same humility. My tone isn’t aggressive, you perceive it as such. I’m not thankful for tone-policing that portrays such subjective matters as a fact.

It seems we’re not making progress here, so I’ll take a step back from this conversation for now.

metavar="table.name",
type=parse_ident,
default=[],
action="append",
help="do not collapse the given table.name (can be specified multiple times)",
)

@property
def override_cli_from_section(self) -> tuple[str, ...]:
""":return: path where config overrides live"""
Expand All @@ -93,6 +103,7 @@ def format(self, text: str, opt: PyProjectFmtNamespace) -> str: # noqa: PLR6301
max_supported_python=opt.max_supported_python,
min_supported_python=(3, 10), # default for when the user didn't specify via requires-python
generate_python_version_classifiers=opt.generate_python_version_classifiers,
do_not_collapse=opt.do_not_collapse,
)
return format_toml(text, settings)

Expand Down
2 changes: 2 additions & 0 deletions pyproject-fmt/src/pyproject_fmt/_lib.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class Settings:
max_supported_python: tuple[int, int],
min_supported_python: tuple[int, int],
generate_python_version_classifiers: bool,
do_not_collapse: list[tuple[str, ...]],
) -> None: ...
@property
def column_width(self) -> int: ...
Expand All @@ -21,3 +22,4 @@ class Settings:
def min_supported_python(self) -> tuple[int, int]: ...

def format_toml(content: str, settings: Settings) -> str: ...
def parse_ident(ident: str) -> tuple[str, ...]: ...
28 changes: 27 additions & 1 deletion pyproject-fmt/tests/test_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import pytest

from pyproject_fmt._lib import Settings, format_toml
from pyproject_fmt._lib import Settings, format_toml, parse_ident


@pytest.mark.parametrize(
Expand Down Expand Up @@ -76,6 +76,32 @@ def test_format_toml(start: str, expected: str) -> None:
min_supported_python=(3, 7),
max_supported_python=(3, 8),
generate_python_version_classifiers=True,
do_not_collapse=[],
)
res = format_toml(dedent(start), settings)
assert res == dedent(expected)


@pytest.mark.parametrize(
("arg", "expected"),
[
("a.b", ("a", "b")),
("a.'b.c'", ("a", "b.c")),
],
)
def test_parse_idents(arg: str, expected: tuple[str, ...]) -> None:
assert parse_ident(arg) == expected


@pytest.mark.parametrize(
("arg", "exc_cls", "msg_pat"),
[
(None, TypeError, r"None"),
("1 b", ValueError, r"syntax error"),
("[]", ValueError, r"syntax error"),
("x.", ValueError, r"syntax error"),
],
)
def test_parse_idents_errors(arg: object, exc_cls: type[Exception], msg_pat: str) -> None:
with pytest.raises(exc_cls, match=msg_pat):
parse_ident(arg)
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ lint.select = [
"ALL",
]
lint.ignore = [
"ANN101", # no type annotation for self
"ANN401", # allow Any as type annotation
"COM812", # Conflict with formatter
"CPY", # No copyright statements
Expand Down
Loading