👌 Parse link conditions from imported and external needs

[chrisjsewell]

Summary

NeedLink.from_string now parses condition syntax (e.g. "REQ_001[status==\"open\"]") from link strings, so that needs loaded via needimport or needs_external_needs can carry conditional links — previously only directive option parsing supported this.

Changes

Core: NeedLink (sphinx_needs/need_item.py)

• from_string — now delegates to from_string_with_warnings, which performs bracket-depth condition parsing (matching the logic previously only in _parse_link_with_condition).
• from_string_with_warnings — new static method returning tuple[NeedLink, list[str]]. Infallible best-effort parse with structured warnings for unclosed brackets or trailing text.
• _parse_address — extracted helper for splitting ID.part addresses.
• to_link_string — serializes a NeedLink back to string including the condition. Computes bracket depth as one more than the longest consecutive ] run in the condition, ensuring safe round-tripping for any condition content.
• to_filter_string — unchanged (no condition, used for filter expressions and JSON output).

Schema: _split_link_list (sphinx_needs/needs_schema.py)

• _parse_link_with_condition removed — its 3-line body (delegate to NeedLink.from_string_with_warnings) is now inlined into _flush_current() inside _split_link_list.

Import prefix fix (sphinx_needs/utils.py)

• import_prefix_link_edit — previously compared raw link strings with id == link, which broke with conditioned links. Now parses each link via NeedLink.from_string, compares .id, and reconstructs with to_link_string() to preserve conditions through the prefixing process.

Documentation

• docs/directives/need.rst — note in the "conditional links" section about JSON support via import/external.
• docs/directives/needimport.rst — note cross-referencing need_conditional_links.
• docs/configuration.rst — note in the needs_external_needs section.

Tests

• tests/test_need_item_api.py — 23 unit tests across TestNeedLinkFromString (11) and TestNeedLinkToLinkString (12), covering plain IDs, parts, conditions, bracket escaping, malformed input, and round-trip serialization.
• tests/test_link_conditions.py — integration test extended to verify warnings from imported needs (needimport with needs_test_conditions.json) and external needs (needs_external_needs with needs_test_external.json).
• New fixtures: needs_test_conditions.json, needs_test_external.json — JSON files with conditioned links for import/external testing.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.03%. Comparing base (4e10030) to head (6253c64).
⚠️ Report is 268 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1680      +/-   ##
==========================================
+ Coverage   86.87%   89.03%   +2.15%     
==========================================
  Files          56       71      +15     
  Lines        6532    10157    +3625     
==========================================
+ Hits         5675     9043    +3368     
- Misses        857     1114     +257     

Flag	Coverage Δ
pytests	89.03% <100.00%> (+2.15%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chrisjsewell]

Overview

Author	@chrisjsewell (member)
Branch	conditions-in-needs-json → master
Stats	+345 / −65 across 12 files
Status	Open, mergeable, not draft

This PR closes a feature gap: conditional link syntax (e.g. REQ_001[status=="open"]) previously only worked when parsed from directive options in .rst files. Now it also works for needs loaded via needimport or needs_external_needs from JSON files.

---

Architecture & Design

The approach is well-structured:

1. Consolidation of parsing logic into NeedLink — The bracket-depth condition parsing that previously lived only in _parse_link_with_condition (in needs_schema.py) is moved into NeedLink.from_string_with_warnings on the model class. This is a good single-responsibility move — the link model now owns its own serialization/deserialization.

2. from_string / from_string_with_warnings split — Clean API: from_string is infallible (best-effort fallback), while from_string_with_warnings surfaces structured warnings. Callers that need diagnostics (like _split_link_list) use the warnings variant; callers that just need a parsed link use the simple version.

3. to_link_string with automatic bracket depth — The serialization method computes the minimum safe bracket depth by scanning for the longest consecutive ] run in the condition, ensuring round-trip safety. This is clever and correctly handles edge cases.

4. Removal of _parse_link_with_condition — The 48-line standalone function is cleanly replaced by a 3-line delegation to the new NeedLink methods. Net deletion of ~65 lines while adding more capability.

---

Code Quality

Strengths

• Excellent docstrings — Every new method has clear documentation of behavior, formats, and edge cases.
• Defensive/infallible parsing — from_string never raises, always returns a best-effort NeedLink. Error paths are well-defined (unclosed brackets → whole string as ID; trailing text → address-only, no condition).
• _parse_address helper — Good extraction of the repeated ID.part splitting logic.

Observations & Minor Points

1. import_prefix_link_edit restructuring (utils.py lines 264–282):

   • The old code had an O(needs × ids × link_names × links) nested loop. The new code reduces this to O(needs × link_names × links) for the link-editing part by parsing the link and checking parsed.id in needs_ids (with needs_ids now a set). This is a nice performance improvement.
   • However, the description-manipulation loop (for id in needs_ids) is still O(needs × ids) with string replacement — this is unchanged and has an existing TODO for regex improvement.

2. from_string_with_warnings only reports the first warning — In _flush_current() in needs_schema.py, warnings[0] is used. Since the parser can only produce one warning per link string (either unclosed or trailing, not both), this is fine, but it's worth noting for future extensibility.

3. bracket_start <= 0 check (line 275 in need_item.py): This uses <= 0 rather than < 0 or == -1. This means a string starting with [ (i.e. bracket_start == 0) is treated as "no address before bracket" and parsed as a plain ID. This is intentional — you need at least one character for the ID before the bracket. Good edge case handling.

4. The to_link_string bracket depth logic — The character-by-character scan is straightforward but could alternatively use a regex like max(len(m) for m in re.findall(r'\]+', condition)). The current approach is arguably more readable and avoids regex overhead for what's usually a short string.

---

Test Coverage

Very thorough:

• 23 unit tests in test_need_item_api.py:

  • TestNeedLinkFromString (11 tests): plain ID, ID.part, conditions, nested brackets, empty conditions, unclosed brackets, trailing text, and the warnings API.
  • TestNeedLinkToLinkString (12 tests): serialization, bracket escaping, and critically round-trip tests that verify from_string(to_link_string(link)) == link for all variants.

• Integration tests extended in test_link_conditions.py:

  • New JSON fixtures (needs_test_conditions.json, needs_test_external.json) with pass/fail/no-condition scenarios.
  • Verifies exact warning messages for both imported and external needs with failing conditions.
  • Tests are ordered alphabetically by warning source, matching the sorted warning output.

---

Documentation

• Notes added in three appropriate places: need.rst (conditional links section), needimport.rst, and configuration.rst (external needs section).
• All notes cross-reference the existing need_conditional_links label and include JSON syntax examples.

---

Potential Concerns

1. Backward compatibility: The from_string method previously only handled ID and ID.part. Now it also parses ID[condition]. For strings that don't contain brackets, behavior is identical. For strings that do (which previously would have been treated as literal IDs), conditions are now extracted. Since this is the desired new behavior and the old code path never encountered bracket syntax from JSON imports, this should be safe.

2. Only warnings[0] is surfaced: If the parser is ever extended to produce multiple warnings per link, the current callers would silently drop extras. Low risk for now.

---

Verdict

This is a clean, well-tested, well-documented PR that correctly consolidates link condition parsing into the NeedLink model, fixes a real limitation (conditional links in JSON imports), and includes both comprehensive unit tests and integration tests. The code is a net simplification (removing duplication) while adding capability.

Looks good to merge ✅