Skip to content

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Oct 10, 2025

The goal of this PR is to enable co-location of all field properties.

Co-location makes it easier for users to understand, and potentially allows for reuse of fields across projects.

Previously this was not possible, for field properties that could be applied to core fields, such as needs_variant_options.

This changes the config from e.g.

needs_extra_options = [
     "extra"
]
needs_extra_links = [
     {"option": "links"}
]
needs_variant_options = ["status", "extra", "links"]

to

needs_core_options = {
    "status": {"variant_functions": True, "dynamic_functions": True}
}
needs_extra_options = [
    {"name": "author", "variant_functions": True, "dynamic_functions": True}
]
needs_extra_links = [
     {"option": "links", "variant_functions": True, "dynamic_functions": True}
]

Note also, this allows for the previously disallowed variants on links, and allows for equivalently allowing/disallowing dynamic functions

In follow-up PRs, it would be the goal to also deprecate needs_global_options, needs_statuses and needs_tags
(i.e. co-locate defaults and enums)


TODO documentation, more tests

Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.00%. Comparing base (4e10030) to head (7268926).
⚠️ Report is 180 commits behind head on master.

Files with missing lines Patch % Lines
sphinx_needs/needs.py 80.00% 5 Missing ⚠️
sphinx_needs/config.py 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
+ Coverage   86.87%   88.00%   +1.12%     
==========================================
  Files          56       70      +14     
  Lines        6532     9645    +3113     
==========================================
+ Hits         5675     8488    +2813     
- Misses        857     1157     +300     
Flag Coverage Δ
pytests 88.00% <85.71%> (+1.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.

🚀 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.

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.

I like that change. It gives user control over what is allowed for the fields.
Is variant_functions a good name? Variants can be used without functions. Maybe variant_definitions?

The settings for core fields just overwrite defaults, right? So status will exist also when not mentioned in that config.

@danwos
Copy link
Member

danwos commented Oct 11, 2025

Like it to have one place to configure everything related to a field.

Another name proposal for variant_functions, as it is not really function: variant_support.

The only thing which may be confusing to the user are the two config options:
needs_core_options and needs_extra_options.
From user perspective, they do the same and look the same.
And how to know, which one to use for a specific field?
Can't we just have needs_options?
And warn/break, if a core option gets redefined?

@chrisjsewell
Copy link
Member Author

Can't we just have needs_options?

Heya @danwos, yeh actually this was my first thought.
It would though take some work in the internal code base, to change where the needs_extra_options is being used to get keys only for extra options,
and also checking that core options are never given with the "wrong" schema type (e.g. number instead of string),
and it would require that all sphinx-needs users update their config (initially, emitting a deprecation warning)

If you and @ubmarco are ok with this, then I would take the time to do it?

Personally, I would like to name it needs_fields

@ubmarco
Copy link
Member

ubmarco commented Oct 16, 2025

I vote for needs_fields. The topic is actually a bit bigger and we should clean things up if we touch the config for this now.

needs_fields encapsulates the current needs_extra_options, needs_extra_links and needs_variant_options and needs_global_options (better name is defaults).

We should also deprecate needs_statuses as this can be done now with

[[needs.extra_options]]
name = "status"
schema.type = "string"
schema.enum = ['open', 'closed']

Same is true for needs_tags which can be given as

[[needs.extra_options]]
name = "tags"
schema.type = "array"
schema.items = { type = "string", enum = ["component1", "component2"] }

Only thing we lose is the description per value info, but that is not used anywhere in the code base.
The info can either go to the field description in plain text, or for string enums we could add a list of descriptions if that is wanted. But I want to see a usecase first. If it is just for code documentation, people can also add a TOML/Python code comment.

Then SN comes with optional defaults for needs_types (req, spec, impl, test, need), core options (tags, status) needs_extra_links (links) and needgantt (options completion, duration are added). We should have a flag needs_add_defaults to deactivate this behavior. That config may default to true.

@danwos
Copy link
Member

danwos commented Oct 16, 2025

I don't like needs_fields in the Sphinx context, as Sphinx itself is talking about directive options and "field" is used in field lists.

Our docs also talk about options everywhere. So needs_fields could become quite confusing.

I would stay with needs_options.

@chrisjsewell
Copy link
Member Author

as Sphinx itself is talking about directive options

I would say that is a bit misleading though, because these are not just directive options, they are fields on a need item, (essentially columns of a database, with needs being the rows) that can come from different places: the title field comes from the argument of the directive, needs themselves can come from external/imported needs.json, generated from codelinks, etc

@ubmarco
Copy link
Member

ubmarco commented Oct 16, 2025

While I don't have a strong opinion on needs_fields vs needs_options I always thought directives options are field lists actually. Or they are implemented as such.
@chrisjsewell sees the name field interchangeably to property, just like a class can have fields.
@danwos also has a point because the term option is well known in SN over the years.

@ubmarco
Copy link
Member

ubmarco commented Oct 17, 2025

By the way, https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#directives says

Directive options are indicated using field lists; the field names and contents are directive-specific.

and in the link

Field lists are used as part of an extension syntax, such as options for directives, or database-like records meant for further processing. They may also be used for two-column table-like structures resembling database records (label & data pairs).

Docutils also specifies option-lists which documents CLI program options.

From another perspective, the term options indicate things being optional. Which it can or cannot be. Field would be the more general term for what we want to specify.

@patdhlk
Copy link

patdhlk commented Oct 17, 2025

My two cents on needs_fields vs needs_options: I would prefer needs_fields, reasoning: For me, links are not necessarily options but something else. Especially when talking to customers about them. needs_fields is more consistent/covers a wider range ☂️

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.

4 participants