Adding language binding for golang#1483
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
kriswest
left a comment
There was a problem hiding this comment.
Looks like a great contribution @kemerava 👏
I've given it a quick pass for typos and indentation consistency, see comments.
I think we could do with a subsection adding here on Go: https://deploy-preview-1483--fdc3.netlify.app/docs/next/api/supported-platforms#native
Please describe any idiosyncracies specific to the language. If you intend to contribute a repo with the binding to FINOS it can also be mentioned there.
docs/api/ref/Types.md
Outdated
| Type string `json:"type"` | ||
| Name string `json:"name"` | ||
| Id map[string]string `json:"id"` | ||
| // This field is required only in golang, as golang structs cannot be extended with more fields later |
There was a problem hiding this comment.
Thank you so much @kriswest for the review! (Sorry about the indentation madness, looks like this new IDE does not do spell checks or any indents, should be better now). Also I have added a section about go in the supported platforms, let me know if this is the level of granularity that would be okay there. Thanks again!
There was a problem hiding this comment.
@kriswest I modified the definitions of Context, this will provide a better support for json of context looking like it is supposed to, without modifying it and also it would make it possible to create other types of contexts (rather than just adding the other fields as part of the map in Context type. However, now it is a bit awkward with having all Context types implement one function. This is just the trade off that had to be made because of go's peculiarities
Co-authored-by: Kris West <kris.west@interop.io>
Co-authored-by: Kris West <kris.west@interop.io>
Co-authored-by: Kris West <kris.west@interop.io>
kriswest
left a comment
There was a problem hiding this comment.
Spotted a small issue rendering the code blocks on the supported platforms page
Co-authored-by: Kris West <kris.west@interop.io>
Thanks for the spot @kriswest! Fixed now! |
|
/netlify |
|
@kriswest could you please review the changes with experimental tag. Thanks! |
|
Closing to reopen in case that fixes the preview issues |
|
@TheJuanAndOnly99 can you ask netlify to rebuild this preview? I don't think it accepts commands from me and closing/reopening didn't affect it (did re-run the github actions however). |
|
@TheJuanAndOnly99 Ideally I'm after it using your later config so it gets a preview URL i can view ;-) But I'd settle for it just successfully building |
kriswest
left a comment
There was a problem hiding this comment.
The changelog needs adjusting after the 2.2 release. Otherwise have suggested a few places to put @experimental.
Will be able to review properly once we get the preview fixed!
|
Hi @kriswest I triggered the preview build https://app.netlify.com/sites/fdc3/deploys/681b9118a04a5583475bab9a. It's still failing here. I think @kemerava will need to pull the changes to this branch as it does not have the updates to the .nvmrc file from main. Once you do so let me know and I can trigger the preview build again. |
|
@kemerava try merging main and then I think we may have ended up with bad checksum in the package-lock.json at some point (probably a bad merge), as there have been a few PRs with this error. |
Co-authored-by: Kris West <kris.west@interop.io>
@kriswest ran both, and there were some changes in the package-lock, so I committed. Hopefully that resolves it |
|
@kemereva that did indeed fix the build, but there's a syntax error in the markdown to sort: https://app.netlify.com/sites/fdc3/deploys/681cd79fbf8e800009f1052a#L423 |
|
@kriswest thanks for noticing, I fixed those errors, now the build is no longer failing |
kriswest
left a comment
There was a problem hiding this comment.
LGTM
@hughtroeger or @novavi could you take a look?
hughtroeger
left a comment
There was a problem hiding this comment.
Well I don't know Go, but all the examples in the site previews look good and I didn't see any typos or other issues. 👍
|
Is there a Go library that implements the FDC3 schema and code behind the documentation in this PR? Sorry if that's an uneducated question. |
|
@chriscasola Not at present, we just have the API binding. However, @kemerava previously indicated a willingness to provide one or collaborate on one in future if there was interest in the Go binding. If you are interested I'd recommend raising an issue. |
Hi @chriscasola , there is not a library implementation at the moment. What are you looking for? The full implementation underneath the API or a library with the interfaces that can be pulled and implemented? |
Describe your change
Adding golang language binding
@michael-bowen-sc
Review deeplinks:
Related Issue
#1482
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