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

updating definition of otherConfig in fdc3.chart to use an array of context objects #985

Conversation

kriswest
Copy link
Contributor

resolves #856

Changes the definition of otherConfig in fdc3.chart from an unstandardized object to an array of Context objects. This allows the type of each additional piece of config to be checked and either used or ignored by a charting application, based on its type.

@rbruckheimer

@kriswest kriswest added context-data FDC3 Context Data Working Group Context Data & Intents Contexts & Intents Discussion Group labels May 11, 2023
@kriswest kriswest added this to the 2.1 milestone May 11, 2023
@kriswest kriswest requested review from mistryvinay and a team May 11, 2023 16:22
@netlify
Copy link

netlify bot commented May 11, 2023

Deploy Preview for fdc3 canceled.

Name Link
🔨 Latest commit 3a43bba
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/64625244ec96510008a61a54

@rbruckheimer
Copy link

Is there a concern for backwards compatibility here?

@kriswest
Copy link
Contributor Author

@rbruckheimer AFAIK ChartIQ is the main user of this type (although my information on that is not perfect), so you'd know better than I. Do you have clients using it currently?

I wouldn't propose a change like this for an element that was standardized, however, this field was specifically for unstandardized data. A backwards-compatible change, that also doesn't have a redundant layer would be to use an object and create fields under it which are arrays, but divide them up by type, e.g. indicators, drawings, formulae, other (so that they have a purpose in organizing the config). OR you could keep otherConfig as a deprecated field and create a proprietaryConfig or customConfig field to supercede it. OR just live with that redundancy (otherConfig.config).

Writing it out, seems like either the breaking change or deprecation and creating a new field seem like the best options.

Let me know what you prefer and I can adjust the PR.

@rbruckheimer
Copy link

Let's go with your proposal and we can change accordingly.

CHANGELOG.md Outdated Show resolved Hide resolved
@kriswest kriswest removed this from the 2.1 milestone May 17, 2023
@kriswest kriswest changed the base branch from master to context-data-and-intents-consolidated-update-branch-2.1 May 17, 2023 15:20
@kriswest
Copy link
Contributor Author

@mistryvinay this should be ready for your review and merge into your consolidated update branch for Context and Intents

@kriswest
Copy link
Contributor Author

@mistryvinay actually looks like I've got small git snafu after changing the base, will fix.

@kriswest kriswest force-pushed the 856-chart-otherconfig-improvement branch from 42419cf to 3a43bba Compare May 17, 2023 15:29
@kriswest
Copy link
Contributor Author

fixed and good to go @mistryvinay.
Needs @greyseer256's blessing too.

Copy link
Contributor

@mistryvinay mistryvinay left a comment

Choose a reason for hiding this comment

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

LGTM

@mistryvinay mistryvinay changed the base branch from context-data-and-intents-consolidated-update-branch-2.1 to fix-context-data-and-intents-consolidated-update-branch-2.1 May 18, 2023 14:01
…-2.1' into 856-chart-otherconfig-improvement
@kriswest
Copy link
Contributor Author

@greyseer256 please check this one when you have a second, so we can merge into the consolidated Context & Intents branch for 2.1

@kriswest kriswest mentioned this pull request May 25, 2023
27 tasks
@kriswest kriswest merged commit fdbd1c5 into fix-context-data-and-intents-consolidated-update-branch-2.1 May 30, 2023
@kriswest kriswest deleted the 856-chart-otherconfig-improvement branch May 30, 2023 10:12
@bingenito bingenito mentioned this pull request Nov 6, 2023
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Context Data & Intents Contexts & Intents Discussion Group context-data FDC3 Context Data Working Group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tighter definition for "otherConfig" in fdc3.chart context type
5 participants