-
Notifications
You must be signed in to change notification settings - Fork 1
EAGLE-1599: Remove 'defaultValue' attribute from Field class #978
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
Reviewer's GuideThis PR removes the legacy “defaultValue” attribute from the Field model, UI, data schema, and tests—simplifying parameter editing to operate solely on current values—and extracts a constant for the “appclass” field name. Class diagram for Field class after removing 'defaultValue' attributeclassDiagram
class Field {
-displayText : ko.Observable<string>
-value : ko.Observable<string>
-description : ko.Observable<string>
-readonly : ko.Observable<boolean>
-type : ko.Observable<Daliuge.DataType>
-precious : ko.Observable<boolean>
-options : ko.ObservableArray<string>
-positional : ko.Observable<boolean>
-parameterType : ko.Observable<Daliuge.FieldType>
-usage : ko.Observable<Daliuge.FieldUsage>
-encoding : ko.Observable<string>
-isEvent : ko.Observable<boolean>
-node : Node
-issues : ko.ObservableArray<{issue:Errors.Issue, validity:Errors.Validity}>
+constructor(id, displayText, value, description, readonly, type, precious, options, positional, parameterType, usage)
+getValue() : string
+setValue(value: string) : Field
+getDescription() : string
+setDescription(value: string) : Field
+getType() : Daliuge.DataType
+setType(type: Daliuge.DataType) : Field
+getOptions() : string[]
+addOption(option: string) : Field
+removeOption(index: number) : Field
+editOption(optionIndex: number, newVal: string) : Field
+clear() : Field
+shallowCopy() : Field
+getFieldValue() : string
+copyWithIds(src: Field, node: Node, id: FieldId) : Field
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- After removing defaultValue from the graph schema, please bump the schema version (e.g. to v5) so consumers can detect the change.
- Add fallback logic when loading older graphs that still include defaultValue to ensure smooth upgrades and avoid missing-attribute errors.
- The new APP_CLASS constant is placed in the DataType enum but is actually a field name; consider moving it into the FieldName enum for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- After removing defaultValue from the graph schema, please bump the schema version (e.g. to v5) so consumers can detect the change.
- Add fallback logic when loading older graphs that still include defaultValue to ensure smooth upgrades and avoid missing-attribute errors.
- The new APP_CLASS constant is placed in the DataType enum but is actually a field name; consider moving it into the FieldName enum for clarity.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
M-Wicenec
left a comment
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.
from a code and bug perspective i found no issues and this pr is ready to merge
one question to discuss is if there are any graphs where the default values where seriously used. removing default value capability from such a graph would mean loss of data. is this a concern?
another question is if there are any further changes necessary on the daliuge side? if any likely just for tests or updates to palettes/ graphs.
Updated ParameterTable UI to remove 'defaultValue' column.
Removed 'resetToDefault' functionality, this is replaced by just removing the Field from the GraphConfig.
Removed the 'defaultValue' attribute from the LG graph schema (V4).
Updated unit test LogicalGraph JSON to remove the 'defaultValue' attribute.
Removed 'defaultValue' inputs from the 'edit field' modal dialog.
One unrelated change where I created a constant for the string name of the 'appclass' field. It is used in one place.
Summary by Sourcery
Remove the 'defaultValue' attribute and associated functionality from the Field class, UI, schema, and tests, replacing resetToDefault with field removal in GraphConfig and adding a constant for the 'appclass' field name.
Enhancements:
Tests:
Chores: