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

✨ Add ndf role, deprecate need_func & [[...]] in need content #1269

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Sep 3, 2024

There are currently two ways to use dynamic functions within need directive's content:

  1. [[copy("id")]]; is problematic, because it does not adhere to the rst/myst syntax specification, and has already show to cause issues and be surprising to users (🐛 FIX disallow dynamic functions [[..]] in literal content #1263)

  2. :need_func:`[[copy("id")]]`; is better but overly verbose

In this PR, I introduce a new role, which I believe should supersede both: :ndf:`copy("id")` (open to other suggestions for the name)
Here we take the entire content to be the function, as so do not require the [[]], reducing verbosity and processing

Note, one thing that the role approach cannot do, is to allow placeholders in URLs, e.g. `link <http://www.[[copy('id')]]>`_
However, if this is truly required, then I believe we should introduce a new role specifically for that.


if this is accepted, I would then move to emit warnings if users already use [[...]], explaining how to replace them,
then eventually remove [[...]] after some deprecation period


things to clarify:

  • do we agree this role should be added
  • do we agree on its name
  • do we agree this should deprecate [[...]] in content
    • do we need an additional role for URL links with placeholders
  • do we agree this should deprecate need_func

@chrisjsewell chrisjsewell changed the title ✨ Add ndf role ✨ Add ndf role (to replace [[...]] in content) Sep 3, 2024
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

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

Project coverage is 87.00%. Comparing base (4e10030) to head (13fef98).
Report is 91 commits behind head on master.

Files with missing lines Patch % Lines
sphinx_needs/roles/need_func.py 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   86.87%   87.00%   +0.12%     
==========================================
  Files          56       60       +4     
  Lines        6532     6970     +438     
==========================================
+ Hits         5675     6064     +389     
- Misses        857      906      +49     
Flag Coverage Δ
pytests 87.00% <94.28%> (+0.12%) ⬆️

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.

@chrisjsewell chrisjsewell changed the title ✨ Add ndf role (to replace [[...]] in content) ✨ Add ndf role (to replace [[...]] in need content) Sep 3, 2024
@chrisjsewell chrisjsewell marked this pull request as draft September 4, 2024 18:09
@chrisjsewell chrisjsewell force-pushed the new_need_func branch 2 times, most recently from 9cb3a42 to 1103fc5 Compare September 6, 2024 10:51
@chrisjsewell chrisjsewell marked this pull request as ready for review September 6, 2024 10:52
@ubmarco
Copy link
Member

ubmarco commented Sep 17, 2024

Hi @chrisjsewell, thanks for this proposal.
As you said and as the past showed, the [[ ]] construct is brittle and breaks RST.
I like the role approach and also the name. Here are my answers:

  • do we agree this role should be added
  • do we agree on its name
  • do we agree this should deprecate [[...]] in content
    • do we need an additional role for URL links with placeholders
  • do we agree this should deprecate need_func

For the URL links I'm hesitant to add it without reported use case.
For ndf - can we somehow make sure that the name does not interfere with other extensions providing a role of the same name? need_func was certainly more scoped to SN.

@chrisjsewell
Copy link
Member Author

Yeh cheers

can we somehow make sure that the name does not interfere

Eurgh, I would be very surprised if someone else was specifically using ndf 😅
Currently it will just override any existing role.
Happy to think of an even more specific name, but the idea was to make it not too verbose, similar to np

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.

LGTM in general, docs and still deprecation missing, but can be done in an upcoming PR

@@ -37,4 +39,6 @@ DYNAMIC FUNCTIONS

nested id also :need_func:`[[copy("id")]]`
Copy link
Member

Choose a reason for hiding this comment

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

if need_func is deprecated, please add a check for the warning in the test case

Copy link
Member

@danwos danwos left a comment

Choose a reason for hiding this comment

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

I'm okay with the changes. Good work.

But as this is a not backward-compatible change, we should add the docs and a depreciation warning for need_fuc with this PR.

@chrisjsewell
Copy link
Member Author

But as this is a not backward-compatible change

yep I was already about to add docs and deprecations 👍 , but just to note, this PR on its own is backwards compatible; it is just adding a new role

@chrisjsewell chrisjsewell requested a review from danwos September 17, 2024 23:52
Copy link
Member

@danwos danwos left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the doc updates.

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.

Nice work and good addition to SN.

@chrisjsewell chrisjsewell changed the title ✨ Add ndf role (to replace [[...]] in need content) ✨ Add ndf role, deprecate need_func & [[...]] in need content Sep 18, 2024
@chrisjsewell chrisjsewell merged commit 976dcd5 into master Sep 18, 2024
18 checks passed
@chrisjsewell chrisjsewell deleted the new_need_func branch September 18, 2024 08:00
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.

3 participants