👌 Capture only expressions for each need
#1112
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1112 +/- ##
==========================================
+ Coverage 83.93% 83.96% +0.02%
==========================================
Files 56 56
Lines 6462 6473 +11
==========================================
+ Hits 5424 5435 +11
Misses 1038 1038
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I see that the 
|
|
Hello, I first tried as you said but I was annoyed that I'd have to take this new field into account in each directive (e.g. needtable) to exclude the need automatically or that users would have to manually exclude the needs when using the directives. About the post-processing, do you mean I have to implement my own logic in my own document ? Or do you suggest we add to this PR an exclusion of the needs in post_process_needs_data ? |
| while parent := getattr(parent, "parent", None): # type: ignore | ||
| if isinstance(parent, addnodes.only): # noqa: SIM102 | ||
| if expr := parent.get("expr"): | ||
| expressions.append(expr) |
There was a problem hiding this comment.
Shouldn't we add a break here, so that we don't "look up" anymore after only was found?
There was a problem hiding this comment.
no; we want to capture ALL parent only expressions, for example (this is in the test case):
.. only:: not something
.. only:: other
.. req:: Requirement 4
:id: REQ_4here the requirement should only be kept if BOTH "not something" and "other" evaluate to true,
so we capture and store {"only_expressions": ["not something", "other"]}
|
I like the PR from a technical point of view, but I'm not a fan of it from a usability point of view. I agree with @David-Le-Nir, that adding this to filter-strings and Co, to get a correct result, is not the best way to go. Especially in bigger projects a user may not know that In most of bigger user projects, we decided to always use a full build to get a reliable result for a Sphinx-Needs project, as there can be so much external data and dynamic functions, which do not work well with an incremental build. Not carrying correctly about Removing |
is this in the documentation? If it doesn't, then really thats a quite a big limitation for sphinx-needs
I feel that sphinx-needs in general works quite well with incremental. I am really quite against introducing things like #1106, that inherently break incremental builds
I don't think this is correct; you would remove the needs items (that have negatively-evaluated |
Ahh okay, then I have misunderstood the concept. But the current PR does not include this removal from the needs-list, right?
But if you change or remove a Need object in File A, |
yes and yes; I haven't implemented the removal yet, just wanted to intially show how the "first" phase would work |
if doesn't already, there is no reason why it cannot be; the needtable is rendered in the build (a.k.a post-processing) phase, so will use the updated need data |
but yeh obviously this is a more broad topic that I want to look into It should also be noted that there are different "levels" of incremental builds, for example:
|
@David-Le-Nir and @danwos, as I explained in #1103 (comment), I think this is a better solution for handling need defined within
onlydirectives.In this first "read" phase, we simply just note all the parent
onlyexpressions of the need, storing them on an (optional)only_expressionsfield of the need data item.If desired, in a subsequent "build" post-processing phase, called from the cached data (once per build), you could then evaluate the
only_expressionsand remove needs as a necessary (or do whatever).This logic would go here:
sphinx-needs/sphinx_needs/directives/need.py
Line 385 in 84a5f72
this is more in-line with the logic of the
onlydirective, where everything is cached and parts of the doctree are only removed during the build phase.would supercede #1106
closes #1103