-
Notifications
You must be signed in to change notification settings - Fork 77
✨ Introduce needs_core_options
, deprecate needs_variant_options
#1547
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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.
Like it to have one place to configure everything related to a field. Another name proposal for The only thing which may be confusing to the user are the two config options: |
Heya @danwos, yeh actually this was my first thought. If you and @ubmarco are ok with this, then I would take the time to do it? Personally, I would like to name it |
I vote for
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 Then SN comes with optional defaults for |
I don't like Our docs also talk about options everywhere. So I would stay with |
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 |
While I don't have a strong opinion on |
By the way, https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#directives says
and in the link
Docutils also specifies option-lists which documents CLI program options. From another perspective, the term |
My two cents on |
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.
to
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
andneeds_tags
(i.e. co-locate defaults and enums)
TODO documentation, more tests