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

Adding language binding for golang #1483

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Adding language binding for golang #1483

wants to merge 12 commits into from

Conversation

kemerava
Copy link
Contributor

@kemerava kemerava commented Dec 26, 2024 •
edited by kriswest
Loading

Describe your change

Adding golang language binding
@michael-bowen-sc

Review deeplinks:

Related Issue

#1482

Contributor License Agreement

  • I acknowledge that a contributor license agreement is required and that I have one in place or will seek to put one in place ASAP.

Review Checklist

  • Issue: If a change was made to the FDC3 Standard, was an issue linked above?
  • CHANGELOG: Is a CHANGELOG.md entry included?
  • API changes: Does this PR include changes to any of the FDC3 APIs (DesktopAgent, Channel, PrivateChannel, Listener, Bridging)?
    • Docs & Sources: If yes, were both documentation (/docs) and sources updated?

      JSDoc comments on interfaces and types should be matched to the main documentation in /docs
    • Conformance tests: If yes, are conformance test definitions (/toolbox/fdc3-conformance) still correct and complete?

      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
    • Schemas: If yes, were changes applied to the Bridging and FDC3 for Web protocol schemas?

      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
      • If yes, was code generation (npm run build) run and the results checked in?

        Generated code will be found at /src/api/BrowserTypes.ts and/or /src/bridging/BridgingTypes.ts
  • Context types: Were new Context type schemas created or modified in this PR?
    • Were the field type conventions adhered to?
    • Was the BaseContext schema applied via allOf (as it is in existing types)?
    • Was a title and description provided for all properties defined in the schema?
    • Was at least one example provided?
    • Was code generation (npm run build) run and the results checked in?

      Generated code will be found at /src/context/ContextTypes.ts
  • Intents: Were new Intents created in this PR?

@kemerava kemerava requested a review from a team as a code owner December 26, 2024 18:45
@netlify Netlify
Copy link

netlify bot commented Dec 26, 2024 •
edited
Loading

✅ Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit d83c626
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/6792ac73fd98f500082ad7be
😎 Deploy Preview https://deploy-preview-1483--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.

@kemerava kemerava changed the title Adding launguage binging for golang Adding launguage binding for golang Dec 26, 2024
kriswest
kriswest reviewed Dec 28, 2024
View reviewed changes
docs/api/ref/Channel.md Outdated Show resolved Hide resolved
kriswest
kriswest requested changes Dec 28, 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.

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/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/Metadata.md Outdated Show resolved Hide resolved
docs/api/ref/Metadata.md Outdated Show resolved Hide resolved
docs/api/ref/Metadata.md Outdated Show resolved Hide resolved
docs/api/ref/Types.md Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

check indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

@kriswest kriswest changed the title Adding launguage binding for golang Adding language binding for golang Dec 28, 2024
@kemerava kemerava requested a review from kriswest January 17, 2025 17:56
@kriswest kriswest mentioned this pull request Jan 23, 2025
Open
22 tasks
kriswest
kriswest requested changes Jan 23, 2025
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.

Spotted a small issue rendering the code blocks on the supported platforms page

docs/api/supported-platforms.md Outdated Show resolved Hide resolved
@kemerava
Copy link
Contributor Author

Spotted a small issue rendering the code blocks on the supported platforms page

Thanks for the spot @kriswest! Fixed now!

@kriswest kriswest requested review from kriswest and a team January 23, 2025 20:58
@kriswest kriswest linked an issue Jan 23, 2025 that may be closed by this pull request
Proposal to add Go (GoLang) language binding #1482
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kriswest kriswest Awaiting requested review from kriswest

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

Proposal to add Go (GoLang) language binding
2 participants
@kemerava @kriswest