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 component/foreach helpers #81

Merged
merged 3 commits into from
Oct 16, 2018
Merged

add component/foreach helpers #81

merged 3 commits into from
Oct 16, 2018

Conversation

Stebalien
Copy link
Member

This adds a Component helper type and a ForEach helper method.

The first attempt used an interface but interfaces imply allocation. We really
can't afford to allocate here.

This adds a `Component` helper type and a `ForEach` helper method.

The first attempt used an interface but interfaces imply allocation. We really
can't afford to allocate here.
Opinionated: Paths should start with /, dammit!
@Stebalien
Copy link
Member Author

Note: this changes the behavior of paths. ValueForProtocol(pathProtocol) will now make the path start with a /.

Rational:

  1. Paths should start with /.
  2. This makes the string-to-bytes and bytes-to-string encoders symmetric. In the past, the former used to take a path starting with a / and the latter would spit out a path without the /.

raulk
raulk previously approved these changes Oct 2, 2018
Copy link
Member

@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.

LGTM, useful abstraction.

return []Protocol{c.protocol}
}

func (c *Component) Decapsulate(o Multiaddr) Multiaddr {
Copy link
Member

Choose a reason for hiding this comment

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

I guess Decapsulate doesn't make much sense in the context of a single component, so it becomes equivalent to a Filter operation, i.e. returns itself if there's a match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically. Personally, given the new SplitFunc, I'm not sure how useful decapsulate is in general.

component.go Outdated
// ForEach walks over the multiaddr, component by component.
//
// This function iterates over components *by value* to avoid allocating.
func ForEach(m Multiaddr, cb func(c Component) bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add a Filter function that returns the components for a given protocol?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about filter but I'm adding SplitFirst, SplitLast, SplitFunc, etc. functions.

@Stebalien Stebalien requested a review from a user October 2, 2018 23:04
@Stebalien Stebalien dismissed raulk’s stale review October 4, 2018 16:01

significant changes

@Stebalien Stebalien requested a review from raulk October 4, 2018 16:01
@Stebalien Stebalien requested a review from Kubuxu October 5, 2018 20:28
@Stebalien Stebalien merged commit 0c17b87 into nit/cleanup Oct 16, 2018
@Stebalien Stebalien deleted the feat/component branch October 16, 2018 15:47
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