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

Pubsub multibase encoding #1065

Closed
wants to merge 1 commit into from
Closed

Pubsub multibase encoding #1065

wants to merge 1 commit into from

Conversation

TMoMoreau
Copy link
Contributor

Documenting and clarifying the requirement of multibase encoding when publishing data to a pubsub topic.

Documenting and clarifying the requirement of multibase encoding when publishing data to a pubsub topic.
@TheDiscordian
Copy link
Member

LGTM, I personally really like that you linked to a tool to assist devs in converting their topic string to Base64URL (really handy especially if someone is coming to the docs after wondering why their pub isn't working). It is linked to an external tool, not 100% sure how kosher that is, but I'm sure @johnnymatthews could bless us with his opinion/wisdom 🙏.

Tagging @Annamarie2019 hoping this more complete PR will help alleviate some confusion ❤️.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

This is a good improvement ❤️ , but this markdown file is generated by this github action workflow, so these changes will get overridden when a new release of go-ipfs ships :(

At very minimum, you also need to update https://github.com/ipfs/ipfs-docs/tree/main/tools/http-api-docs generator and special-case this specific endpoint to include this additional text.

FYSA I filled ipfs/go-ipfs-cmds#224 to add a programatic way to indicate which string fields are expected to be multibase – if someone is able to implement that, then generating docs should be way easier, but it is also fine to special-case in tools/http-api-docs for now, if you find it easier.

@TheDiscordian
Copy link
Member

@lidel thank you so much putting us on the correct path here (even with options)!

@TheDiscordian
Copy link
Member

Closed PR as per Lidel's message.

@TMoMoreau TMoMoreau deleted the patch-3 branch November 10, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants