🐛 fix: read the file back before handing it over

Both formatters returned what they wrote without reading it, leaving the
file itself as the first thing to try. Each one parses its output now,
and the tox formatter says why it rejects a file rather than handing
back what it was given.

Whitespace between the clauses of `requires-python` says nothing, while
whitespace inside one is what makes the text something PEP 440 does not
read: `> = 3.10` stays as written rather than becoming a constraint the
file never named, and a field this cannot read leaves the classifiers
beside it alone.

A setting read from a TOML table is held to what the command line would
accept, so a negative count, a boolean, or a `table_format` no formatter
knows is reported against the file that names it. Text `tomllib` cannot
read is left to the formatter to report on, rather than reaching the user
as a traceback from a reader that is only there for the settings.

A list the file wrote as a comment names no environment tox runs, so it
no longer decides where the tables that hold them go.

Author: gaborbernat

common/src/lib.rs

@@ -11,3 +11,21 @@ pub mod pep508;
 pub mod sections;
 pub mod spacing;
 pub mod strings;
+
+/// Hand back what the formatter wrote, once it reads back as the document it is meant to be.
+///
+/// The file the caller holds is the last valid one until this returns, so text no parser accepts
+/// never reaches it.
+///
+/// # Errors
+///
+/// Returns where the written text stops being a document.
+pub fn written_document(written: &str) -> Result<String, String> {
+    match toml_doc::parse(written) {
+        Ok(_) => Ok(written.to_owned()),
+        Err(errors) => Err(format!(
+            "the formatter wrote something no reader accepts: {}",
+            errors[0]
+        )),
+    }
+}

common/src/pep508/version_op.rs

@@ -25,8 +25,12 @@ impl VersionOp {
         if literal.is_empty() {
             return Err(format!("The clause names no version: '{spec}'"));
         }
-        // `===` compares the text it is given, which PEP 440 lets be anything at all
+        // `===` compares the text it is given, which PEP 440 lets be anything a version token can
+        // hold, and whitespace is not one of those
         if op == Operator::ArbitraryEqual {
+            if literal.contains(char::is_whitespace) {
+                return Err(format!("The version names more than one word: '{literal}'"));
+            }
             return Ok(Self {
                 version: Version::new(&literal).ok(),
                 op,

common/tests/invariants.rs

@@ -166,3 +166,14 @@ fn reordering_members_leaves_a_document_that_reads_back() {
         }
     });
 }
+
+/// The file the caller holds is the last valid one until the formatter returns, so what it wrote is
+/// read back before it goes anywhere.
+#[test]
+fn what_the_formatter_writes_reads_back_as_a_document() {
+    assert_eq!(
+        common::written_document("a = [1,\n]\n"),
+        Ok(String::from("a = [1,\n]\n"))
+    );
+    assert!(common::written_document("a = [1,\n").is_err());
+}

common/tests/pep508.rs

@@ -530,6 +530,7 @@ fn test_a_version_list_the_parentheses_do_not_close_is_not_a_requirement() {
 #[test]
 fn test_an_operator_and_a_version_pep_440_does_not_pair_is_not_a_requirement() {
     for raw in [
+        "pkg=== foo bar",
         "pkg>=1.*",
         "pkg<=1.*",
         "pkg~=1",

pyproject-fmt/rust/src/main.rs

@@ -171,7 +171,8 @@ pub fn format_toml(content: &str, opt: &Settings) -> Result<String, String> {
     // below hands the formatter a document with its disabled keys turned back on, which may say the
     // same name twice
     toml_doc::parse(content).map_err(|errors| errors[0].to_string())?;
-    common::disabled::try_with_disabled_keys(content, |content| format_core(content, opt))
+    let written = common::disabled::try_with_disabled_keys(content, |content| format_core(content, opt))?;
+    common::written_document(&written)
 }
 
 fn format_core(content: &str, opt: &Settings) -> Result<String, String> {

pyproject-fmt/rust/src/project.rs

@@ -141,7 +141,12 @@ pub fn fix(
             });
         }
         "requires-python" => {
-            common::strings::update(value, |text| text.split_whitespace().collect());
+            // whitespace between the clauses says nothing, while whitespace inside one is what
+            // makes the text something PEP 440 does not read: taking it out would write a
+            // constraint the file does not name
+            common::strings::update(value, |text| {
+                read_specifiers(text).map_or_else(|| text.to_owned(), |_| text.split_whitespace().collect())
+            });
         }
         "dependencies" | "optional-dependencies" => {
             normalize_and_sort_requirements(value, keep_full_version);
@@ -328,6 +333,9 @@ fn generate_classifiers(
     }
     let (minors, existing) =
         supported_minors_with_classifier(&section.entries, max_supported_python, min_supported_python);
+    let Some(minors) = minors else {
+        return;
+    };
     let Some(existing) = existing else {
         // written as something other than a list of classifiers; a second key would say it twice
         if minors.is_empty()
@@ -381,7 +389,7 @@ fn apply_classifiers(array: &mut toml_doc::Array<'_>, minors: &[u8], existing: &
     array.trailing_comma = true;
 }
 
-type MinorsWithClassifier = (Vec<u8>, Option<HashSet<String>>);
+type MinorsWithClassifier = (Option<Vec<u8>>, Option<HashSet<String>>);
 
 /// The Python 3 minor versions the project supports, and the classifiers it already names.
 ///
@@ -410,13 +418,18 @@ fn supported_minors_with_classifier(
         }
     }
     let window = min_supported_python.1..=max_supported_python.1;
-    let Some(clauses) = requires.as_deref().and_then(read_specifiers) else {
-        return (window.collect(), classifiers);
+    let Some(text) = requires else {
+        return (Some(window.collect()), classifiers);
+    };
+    // a constraint this cannot read still says what the project supports, and the configured window
+    // would say something else in its place
+    let Some(clauses) = read_specifiers(&text) else {
+        return (None, classifiers);
     };
     let minors = window
         .filter(|minor| series_holds_a_release(&clauses, *minor))
         .collect();
-    (minors, classifiers)
+    (Some(minors), classifiers)
 }
 
 /// The clauses of a specifier set, or `None` when the text is not one this can read.

pyproject-fmt/rust/src/tests/project_tests.rs

@@ -2233,8 +2233,8 @@ fn test_project_requires_python_reads_the_whole_specifier_set() {
     assert_eq!(minors("===foobar"), named(&[]));
 }
 
-/// Text that is not a specifier set says nothing about what the project supports, so the configured
-/// window stands.
+/// A constraint this cannot read still says what the project supports, so the classifiers beside it
+/// are left as the file wrote them and the text itself is not tidied into something else.
 #[test]
 fn test_project_requires_python_that_is_not_a_specifier() {
     let start = indoc! {r#"
@@ -2248,13 +2248,36 @@ fn test_project_requires_python_that_is_not_a_specifier() {
     [project]
     name = "test"
     requires-python = "three"
+    classifiers = [ "License :: OSI Approved :: MIT License" ]
+    "#);
+}
+
+/// Whitespace between the clauses says nothing, while whitespace inside one is what makes the text
+/// something PEP 440 does not read.
+#[test]
+fn test_project_requires_python_holding_whitespace_inside_a_clause() {
+    let written = |text: &str| {
+        let start = format!("[project]\nname = \"test\"\nrequires-python = \"{text}\"\n");
+        evaluate_project(&start, false, (3, 12), true)
+    };
+
+    insta::assert_snapshot!(written(">= 3.9, < 4"), @r#"
+    [project]
+    name = "test"
+    requires-python = ">=3.9,<4"
     classifiers = [
-      "License :: OSI Approved :: MIT License",
       "Programming Language :: Python :: 3 :: Only",
       "Programming Language :: Python :: 3.9",
       "Programming Language :: Python :: 3.10",
+      "Programming Language :: Python :: 3.11",
+      "Programming Language :: Python :: 3.12",
     ]
     "#);
+    for held in ["> = 3.10", ">=3 . 10", "=== foo bar"] {
+        let result = written(held);
+        assert!(result.contains(&format!("requires-python = \"{held}\"")), "{result}");
+        assert!(!result.contains("Programming Language"), "{result}");
+    }
 }
 
 #[test]

pyproject-fmt/tests/test_main.py

@@ -146,6 +146,31 @@ def test_a_setting_the_formatter_cannot_hold(
     assert message in capsys.readouterr().err
 
 
+@pytest.mark.parametrize(
+    ("setting", "message"),
+    [
+        pytest.param("indent = -1", "must not be negative", id="negative-count"),
+        pytest.param('table_format = "wide"', "invalid choice", id="unknown-table-format"),
+        pytest.param("column_width = true", "invalid count", id="boolean-count"),
+    ],
+)
+def test_a_configured_setting_the_formatter_cannot_hold(
+    tmp_path: Path,
+    capsys: pytest.CaptureFixture[str],
+    setting: str,
+    message: str,
+) -> None:
+    pyproject_toml = tmp_path / "pyproject.toml"
+    start = f'[project]\nname = "x"\n\n[tool.pyproject-fmt]\n{setting}\n'
+    pyproject_toml.write_text(start)
+
+    with pytest.raises(SystemExit):
+        run([str(pyproject_toml)])
+
+    assert message in capsys.readouterr().err
+    assert pyproject_toml.read_text() == start
+
+
 def test_keep_full_version_cli(tmp_path: Path) -> None:
     start = """\
     [build-system]