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.

Sign up for GitHub

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

Jump to bottom

Clarify document while poc fdc3 #1314

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Clarify document while poc fdc3 #1314

merged 5 commits into from
Sep 13, 2024

Conversation

YaoYao-dd
Copy link
Contributor

fix the code error in the PrivateChannel doc.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Aug 14, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify Netlify
Copy link

netlify bot commented Aug 14, 2024 •
edited
Loading

✅ Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit 164762b
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/66e1a4b28e584e000881537d
😎 Deploy Preview https://deploy-preview-1314--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@YaoYao-dd YaoYao-dd mentioned this pull request Aug 20, 2024
Closed
kriswest
kriswest previously approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM - but needs a minor tweak in the changelog.

@YaoYao-dd do you know what you need to do get a CLA in place so that we can merge the PR? Start by clicking on the 'Please click here to be authorized' in the easyCLA comment on the PR. If you have any trouble @robmoffat or help@finos.org should be able to help.

CHANGELOG.md Outdated
@@ -20,6 +20,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
* Added missing `desktopAgent` field to ImplementationMetadata objects returned for all agents connect to a DesktopAgent bridge in Connection Step 6 connectAgentsUpdate messages and refined the schema used to collect this info in step 3 handshake. ([#1177](https://github.com/finos/FDC3/pull/1177))
* Removed the `version` field from `IntentResolution` as there are no version fields for intents in the FDC3 API definitions and hence the field has no purpose. ([#1170](https://github.com/finos/FDC3/pull/1170))

* Fixed the code error with Client-side example from `PrivateChannel` document as 'symbol' is misused, 'ticker' should be used to align with sever-side example, also align with instrument type standard.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Fixed the code error with Client-side example from `PrivateChannel` document as 'symbol' is misused, 'ticker' should be used to align with sever-side example, also align with instrument type standard.
* Fixed an error in the Client-side example from `PrivateChannel` document as 'symbol' is misused, 'ticker' should be used to align with sever-side example, also align with instrument type standard. ([#1314](https://github.com/finos/FDC3/pull/1314))

Add PR link to changelog entry

@kriswest
Copy link
Contributor

/easycla

@kayhern kayhern requested a review from a team as a code owner August 27, 2024 17:32
@bingenito
Copy link
Member

/easycla

@YaoYao-dd
Copy link
Contributor Author

YaoYao-dd commented Aug 28, 2024 •
edited
Loading

LGTM - but needs a minor tweak in the changelog.

@YaoYao-dd do you know what you need to do get a CLA in place so that we can merge the PR? Start by clicking on the 'Please click here to be authorized' in the easyCLA comment on the PR. If you have any trouble @robmoffat or help@finos.org should be able to help.

Thanks, updated the changelog, now trying to pass easyCLA process, waiting for the approval, thanks.

kriswest
kriswest reviewed Sep 9, 2024
View reviewed changes
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM - but I noticed the same error in the addIntentListener example on the DesktopAgent page, that should be fixed at the same time:

FDC3/docs/api/ref/DesktopAgent.md

Line 167 in 71cd5df

const symbol = context.id.symbol;

CHANGELOG.md Outdated
@@ -22,6 +22,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
* Added missing `desktopAgent` field to ImplementationMetadata objects returned for all agents connect to a DesktopAgent bridge in Connection Step 6 connectAgentsUpdate messages and refined the schema used to collect this info in step 3 handshake. ([#1177](https://github.com/finos/FDC3/pull/1177))
* Removed the `version` field from `IntentResolution` as there are no version fields for intents in the FDC3 API definitions and hence the field has no purpose. ([#1170](https://github.com/finos/FDC3/pull/1170))

* Fixed an error in the Client-side example from `PrivateChannel` document as 'symbol' is misused, 'ticker' should be used to align with sever-side example, also align with instrument type standard. ([#1314](https://github.com/finos/FDC3/pull/1314))
Copy link
Contributor

@kriswest kriswest Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps shorten the changelog and correct grammar:

* Fixed an error in the Client-side example from `PrivateChannel` by correcting `id.symbol` to `id.ticker` to align with the `fdc3.instrument` context. ([#1314](https://github.com/finos/FDC3/pull/1314))`

Copy link
Contributor

@kriswest kriswest Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If addIntentListener example is fixed, could change to:

* Fixed error in the Client-side example from `PrivateChannel` and `addIntentListener` by correcting `id.symbol` to `id.ticker` to align with the `fdc3.instrument` context. ([#1314](https://github.com/finos/FDC3/pull/1314))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure do, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done， can you please review, thanks.

@YaoYao-dd
fixed the wrong code example in DesktopAgeng.md too, update changelog
164762b 
Signed-off-by: Yao, Yao <donghong.yao@fmr.com>
kriswest
kriswest approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@YaoYao-dd
Copy link
Contributor Author

Thanks @kriswest ! can you please help to merge the PR, seems I don't have the access, could NOT find the 'merge' button, thanks.

@kriswest kriswest merged commit bb22382 into finos:main Sep 13, 2024
6 checks passed
@kriswest
Copy link
Contributor

@YaoYao-dd all done - apologies I thought I'd done that yesterday!

@YaoYao-dd
Copy link
Contributor Author

@kriswest Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kriswest kriswest kriswest approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@YaoYao-dd @kriswest @bingenito