♻️ Don't process dynamic functions on internal need fields #1387
Conversation
For most "internal" need fields it does not make sense that these would be dynamic, and anyway this would fail since they are not string types. In this PR, we skip dynamic function processing, for core fields that should not be altered by the user.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1387 +/- ##
==========================================
+ Coverage 86.87% 88.20% +1.32%
==========================================
Files 56 60 +4
Lines 6532 7129 +597
==========================================
+ Hits 5675 6288 +613
+ Misses 857 841 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -167,33 +174,38 @@ class CoreFieldParameters(TypedDict): | |||
| "type": { | |||
| "description": "Type of the need.", | |||
| "schema": {"type": "string", "default": ""}, | |||
| "allow_df": True, | |||
There was a problem hiding this comment.
How can the type and below type specific values be set by a df?
There was a problem hiding this comment.
well thats a discussion to be had; I wouldn't expect them to, but they technically can already, so removing them would be a breaking change.
Currently, in this PR I have basically just allowed all str/list[str] fields, so as to be approximately back-compatible
There was a problem hiding this comment.
Let's deprecate this, only usecase are 'type fields' coming from needimport or external needs. Users are in control of the JSONs, so they can set primary data there.
There was a problem hiding this comment.
Generally approve of this as this is something that users shouldn't be doing.
Not sure whether we break anyone's workflow.
I'd deprecate the dynamic functions on core fields, then finally remove that possibility. This PR makes our life simpler by making this configurable. Thanks.
It is unintended and unlikely the dynamic functions would have been used in there fields, but just in case we issue a warning of the deprecation, so that we can remove them in the future fields affected: `type`, `type_name`, `type_prefix`, `type_color`, `type_style`. this follows on from #1387 and the fields now allowed are: - ``status`` - ``tags`` - ``style`` - ``constraints`` - all `needs_extra_options` - all `needs_extra_links` - all `needs_global_options`
For most "internal" need fields it does not make sense that these would be dynamic, and anyway this would fail since their values are not string types.
In this PR, we skip dynamic function processing, for core fields that should not be altered by the user.