✨ Add ids option for needimport
#1292
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1292 +/- ##
==========================================
- Coverage 86.87% 86.86% -0.02%
==========================================
Files 56 60 +4
Lines 6532 6914 +382
==========================================
+ Hits 5675 6006 +331
- Misses 857 908 +51
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
A more performant alternative to the `filter` option. Also, add and to improve the `needimport` tests
chrisjsewell
force-pushed
the
needimport-ids
branch
from
6776e98 to
5d5c061
Compare
| try: | ||
| with open(correct_need_import_path) as needs_file: | ||
| needs_import_list = json.load(needs_file) | ||
| except json.JSONDecodeError as e: |
There was a problem hiding this comment.
OSError should also be caught, but there is already a TODO
There was a problem hiding this comment.
(knowing what exceptions to handle is always a pain, I was reading this with interest recently https://discuss.python.org/t/extend-type-hints-to-cover-exceptions/23788/51)
| @@ -146,6 +143,13 @@ def run(self) -> Sequence[nodes.Node]: | |||
|
|
|||
| needs_config = NeedsSphinxConfig(self.config) | |||
| data = needs_import_list["versions"][version] | |||
|
|
|||
| if ids := self.options.get("ids"): | |||
| id_list = [i.strip() for i in ids.split(",") if i.strip()] | |||
There was a problem hiding this comment.
Can IDs contain a dynamic function, so you would have to use the 'split with dyn' function?
There was a problem hiding this comment.
oh no definitely not, no dynamic functions here; this is intended for fast filtering
| need["tags"] = need["tags"] + tags | ||
| if tags := [ | ||
| tag.strip() | ||
| for tag in re.split("[;,]", self.options.get("tags", "")) |
There was a problem hiding this comment.
same here, the new function should be used (or is that in a different open PR?)
There was a problem hiding this comment.
this was already existing, but again no dynamic functions.
I feel you are confusing this directive with a need declaration directive, that is the only place we use dynamic functions in the directive options
A more performant alternative to the
filteroption.This PR also adds some other performance optimisations;
caching the schema load and not reading the import file twice (during validation)
Also, add to and improve the
needimporttests