feat(context): add instrument classification property #1665
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
hughtroeger
force-pushed
the
instrument-classification
branch
from
9683c0b to
c67cc8e
Compare
| * @experimental classification of the instrument by type or category. SHOULD be one of the | ||
| * standardized values, although other string values are permitted. | ||
| * |
There was a problem hiding this comment.
I feel like something may have gone wrong with the generation here? I'm not sure why this was added in so many places and seems to be associated with interactionType
There was a problem hiding this comment.
I think its been merged with interactionType by quick type as they are both reducing to string.
However, I'm also really confused as to why we're getting multiple /** */ comment blocks with no type code under them... The two things maybe related - perhaps its just not able/willing to generate a type representing just a string but is still outputting the comment??
We should check if this is happening in the initial generation or the post-processing @Roaders developed to apply to it - I suspect in the generation. Its already going on prior to your PR (as your comments are getting added to existing orphaned blocks) @hughtroeger, but I think is something we need to investigate!
There was a problem hiding this comment.
If you look down at the end of the file it seems to think its created types and refers to them, e.g. PurpleInstrumentClassification, TentacledInstrumentClassification etc., but I don't seem them actually created!
There was a problem hiding this comment.
post-processing I mentioned doesn't run on the content types, so issue is definitely in the generation
There was a problem hiding this comment.
I had a poke at whether there is a simple resolution (e.g. adding a title can sometimes help) but without any success. If QuickType was more willing to replicate the structure of the schemas exactly I think we'd be much happier with its output - but without working on quicktype directly (which is not easy!) I think our best option might be to simplify and remove enums and comments on strings and just have the values in the description fields for now (will at least make it into typedocs). It will not make any different to the results of schema validation nor type checks (as it all reduces to string)
I've raised #1667 to demonstrate what that cleans up in the generated code. It doesn't fix the descriptions however.
WDYT @hughtroeger
There was a problem hiding this comment.
That's fine with me @kriswest, I will update this PR to just use type string with the values in the description field.
|
|
|
|
1 similar comment
|
|
| <details> | ||
| <summary><code>classification</code></summary> | ||
|
|
||
| **Any of:** | ||
|
|
||
| - **type**: `string` with values: | ||
| - `commodity`, | ||
| - `commodityIndex`, | ||
| - `corporateDebt`, | ||
| - `creditDefaultSwapIndex`, | ||
| - `deal`, | ||
| - `debt`, | ||
| - `debtIndex`, | ||
| - `etf`, | ||
| - `fixedIncome`, | ||
| - `future`, | ||
| - `governmentBenchmarkDebt`, | ||
| - `loan`, | ||
| - `mortgageBackedSecurity`, | ||
| - `municipalDebt`, | ||
| - `mutualFund`, | ||
| - `mutualFundIndex`, | ||
| - `option`, | ||
| - `otherDebt`, | ||
| - `ownershipPrivateCompany`, | ||
| - `pevcFirm`, | ||
| - `pevcFund`, | ||
| - `privateCompany`, | ||
| - `publicCompany`, | ||
| - `publicCompanyIndex`, | ||
| - `sovereignDebt`, | ||
| - `structuredProduct`, | ||
| - `unknown` | ||
| - **type**: `string` | ||
|
|
||
| [@experimental](/docs/fdc3-compliance#experimental-features) classification of the instrument by type or category. SHOULD be one of the standardized values, although other string values are permitted. | ||
|
|
||
| </details> | ||
|
|
There was a problem hiding this comment.
@kriswest it seems like this file is generated from the schemas by the docs build. I'm assuming I should remove the changes I made manually?
There was a problem hiding this comment.
Yes, its best left to the generator (which will overwrite it when preparing the website). You should cd website; npm run start at least once, which will generate the file and let you proof it locally then check in the generated result.
Our review checklist should really be covering that and isn't. I'll knock up a quick adjustment to it.
There was a problem hiding this comment.
Thanks! I pushed the generated files.
|
|
2 similar comments
|
|
|
|
hughtroeger
force-pushed
the
instrument-classification
branch
from
e56a548 to
c626d52
Compare
|
|
|
As we've moved to an object type rather than a single field, the enum could come back - certainly for FDS_TYPE, but also for name. If someone wanted to use a different set they can give the set a name and use whatever they like... Just thinking out loud |
|
I'm not entirely happy with how the intrument classificaiton is being combined with the organisation classification in the generated Browser types. Looking at why they are getting combined. |
1 similar comment
|
I'm not entirely happy with how the intrument classificaiton is being combined with the organisation classification in the generated Browser types. Looking at why they are getting combined. |
hughtroeger
force-pushed
the
instrument-classification
branch
from
c626d52 to
1e3e1f8
Compare
|
|
|
@kriswest I added the enum for |
|
|
||
| **type**: `string` | ||
|
|
||
| classification of the instrument by type or category. SHOULD be one of the following values, although other string values are permitted: '`commodity`', '`commodityIndex`', '`corporateDebt`', '`creditDefaultSwapIndex`', '`deal`', '`debt`', '`debtIndex`', '`etf`', '`fixedIncome`', '`future`', '`governmentBenchmarkDebt`', '`loan`', '`mortgageBackedSecurity`', '`municipalDebt`', '`mutualFund`', '`mutualFundIndex`', '`option`', '`otherDebt`', '`ownershipPrivateCompany`', '`pevcFirm`', '`pevcFund`', '`privateCompany`', '`publicCompany`', '`publicCompanyIndex`', '`sovereignDebt`', '`structuredProduct`', '`unknown`' |
There was a problem hiding this comment.
| classification of the instrument by type or category. SHOULD be one of the following values, although other string values are permitted: '`commodity`', '`commodityIndex`', '`corporateDebt`', '`creditDefaultSwapIndex`', '`deal`', '`debt`', '`debtIndex`', '`etf`', '`fixedIncome`', '`future`', '`governmentBenchmarkDebt`', '`loan`', '`mortgageBackedSecurity`', '`municipalDebt`', '`mutualFund`', '`mutualFundIndex`', '`option`', '`otherDebt`', '`ownershipPrivateCompany`', '`pevcFirm`', '`pevcFund`', '`privateCompany`', '`publicCompany`', '`publicCompanyIndex`', '`sovereignDebt`', '`structuredProduct`', '`unknown`' | |
| Optional human-readable classification, to be used if no specific data classification is available. |
There was a problem hiding this comment.
I think this is more in line with how we describe name in other objects. It's specifically not an id...
There was a problem hiding this comment.
Thanks @openfin-johans, just to confirm, you think we should leave out the enumerated recommended values here? Because those are more appropriate with the other sub fields like FDS_TYPE?
There was a problem hiding this comment.
@openfin-johans I've updated the files with your suggestion.
This adds an experimental optional classification field to the instrument context. For use in categorizing different types of financial instruments. Fixes #1615
hughtroeger
force-pushed
the
instrument-classification
branch
from
74c0667 to
e4e2024
Compare
|
|
1 similar comment
|
|
This adds an experimental optional classification field to the instrument context. For use in categorizing different types of financial instruments.
Fixes #1615
Describe your change
Related Issue
Contributor License Agreement
Review Checklist
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?JSDoc comments on interfaces and types should be matched to the main documentation in /docs
Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
npm run build) run and the results checked in?Generated code will be found at
/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.tsBaseContextschema applied viaallOf(as it is in existing types)?titleanddescriptionprovided for all properties defined in the schema?npm run build) run and the results checked in?Generated code will be found at
/src/context/ContextTypes.ts