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

add p2p-webrtc-direct pattern #14

Closed
wants to merge 2 commits into from
Closed

add p2p-webrtc-direct pattern #14

wants to merge 2 commits into from

Conversation

backkem
Copy link

@backkem backkem commented Dec 20, 2018

This PR ports the p2p-webrtc-direct pattern from js-mafmt in preparation of a go-libp2p-webrtc-direct transport. It depends on:

I'll probably need some help with the dependency management.

Copy link
Collaborator

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Minor comments. Overall LGTM, although I'm worried about turning mafmt into a *-import module in the long run.

To avoid stalling, I'll commit the changes myself on your branch.

EDIT: I can't because this repo is under whyrusleeping's account rather than in the multiformats org. I'll clone it there, migrate downstream usages in our control, and port over this PR.

patterns.go Outdated
// Define a dns6 format multiaddr
var DNS6 = Base(madns.P_DNS6)

var _DNS = Or(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is copied from js-mafmt, the _ convention usually denotes unexported vars. In go you can lowercase this.

patterns.go Outdated

// Define a dns4 or dns6 format multiaddr
var DNS = Or(
And(_DNS, Base(ma.P_TCP)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is wonky at origin: multiformats/js-mafmt#40

@backkem
Copy link
Author

backkem commented Mar 1, 2019

This was added in multiformats/go-multiaddr-fmt.

@backkem backkem closed this Mar 1, 2019
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.

2 participants