Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔧 Synchronize list splitting behaviour in need and needextend directives #1385

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

chrisjsewell
Copy link
Member

NeedextendDirective had a different function than NeedDirective, for splitting lists from directive options.

This PR combines the two, to make the behaviour consistent.

`NeedextendDirective` had a different function than `NeedDirective`, for splitting lists from directive options.

This PR combines the two, to make the behaviour consistent.
@chrisjsewell chrisjsewell requested a review from ubmarco January 26, 2025 12:59
Copy link

codecov bot commented Jan 26, 2025

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.16%. Comparing base (4e10030) to head (33d41be).
Report is 101 commits behind head on master.

Files with missing lines Patch % Lines
sphinx_needs/api/need.py 93.75% 1 Missing ⚠️
sphinx_needs/directives/needextend.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1385      +/-   ##
==========================================
+ Coverage   86.87%   88.16%   +1.28%     
==========================================
  Files          56       60       +4     
  Lines        6532     7117     +585     
==========================================
+ Hits         5675     6275     +600     
+ Misses        857      842      -15     
Flag Coverage Δ
pytests 88.16% <91.66%> (+1.28%) ⬆️

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.

Copy link
Member

@ubmarco ubmarco left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up.

@@ -767,15 +782,19 @@ def _split_list_with_dyn_funcs(
)
text = text[2:]
elif text[0] in ";|,":
Copy link
Member

Choose a reason for hiding this comment

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

This looks like there are multiple splitting characters. Maybe mention this in https://sphinx-needs.readthedocs.io/en/latest/filter.html#filter-options. The pages currently says For :status:, :tags: and :types: values are separated by “;”.. Have not found another mention of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I would rather there were not these multiple characters, but that would be back breaking 🤷
will look at docs in separate PR

@chrisjsewell chrisjsewell merged commit 5db85af into master Jan 27, 2025
18 checks passed
@chrisjsewell chrisjsewell deleted the needextend-split-str branch January 27, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants