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

Refactor EnumerateChildren to avoid need for bestEffort paramater. #3700

Merged
merged 3 commits into from
Mar 2, 2017

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Feb 16, 2017

License: MIT
Signed-off-by: Kevin Atkinson [email protected]

@kevina kevina added the status/in-progress In progress label Feb 16, 2017
@kevina
Copy link
Contributor Author

kevina commented Feb 16, 2017

WIP. A test is failing locally for me and I want to see if it okay on the build servers.

@kevina kevina force-pushed the kevina/enumerate-children-refactor branch from d5e4a40 to 5bbe4a4 Compare February 16, 2017 22:12
@kevina kevina changed the title WIP: Refactor EnumerateChildren to avoid need for bestEffort paramater. Refactor EnumerateChildren to avoid need for bestEffort paramater. Feb 16, 2017
@kevina
Copy link
Contributor Author

kevina commented Feb 16, 2017

@whyrusleeping okay the tests are passing. Let me know if you are like this approach. If you do I will refactor the Async version in a similar fashion.

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

@kevina this seems like a very reasonable refactor. Lets make sure not to change the function parameter ordering. the new function parameter should take the place of the linkservice

@kevina
Copy link
Contributor Author

kevina commented Feb 17, 2017

Lets make sure not to change the function parameter ordering. the new function parameter should take the place of the linkservice

I only did that because I thought the new order made more sense. Is there a reason you don't like it? Just curious. I have no problem changing them back.

@whyrusleeping
Copy link
Member

@kevina Eh, I think it makes more sense to have the fetcher first, as its almost as if the cid could be an argument to a method on the 'fetcher function'.

@kevina
Copy link
Contributor Author

kevina commented Feb 17, 2017

@kevina Eh, I think it makes more sense to have the fetcher first, as its almost as if the cid could be an argument to a method on the 'fetcher function'.

Okay, will keep the order the same. Will also see what I can do to refactor the Async version, although that will be a little more involved.

@kevina kevina force-pushed the kevina/enumerate-children-refactor branch from 5bbe4a4 to d7af05d Compare February 17, 2017 01:39
This was referenced Feb 17, 2017
@whyrusleeping whyrusleeping added this to the Ipfs 0.4.7 milestone Feb 17, 2017
@kevina kevina mentioned this pull request Feb 20, 2017
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Code LGTM, pointed out a few typos

// calling Get in DAGService
// Return all links for a node. The complete node does not
// necessarily have to exist locally, or at all. For example, raw
// leaves can not possible have links so there is no need to look
Copy link
Member

Choose a reason for hiding this comment

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

typo: cannot possibly

@@ -114,6 +116,8 @@ func decodeBlock(b blocks.Block) (node.Node, error) {
}
}

// GetLinks return the links for the node, the node doesn't necessary
Copy link
Member

Choose a reason for hiding this comment

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

typo: necessarily

@@ -138,9 +142,22 @@ func (n *dagService) Remove(nd node.Node) error {
return n.Blocks.DeleteBlock(nd)
}

// Create a functtion to get the links for a node, from the node,
Copy link
Member

Choose a reason for hiding this comment

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

typo: function, guaranteed

Copy link
Member

Choose a reason for hiding this comment

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

Also go style of docs says that you should start doc comment with the name of the function.

@kevina kevina force-pushed the kevina/enumerate-children-refactor branch from daf983d to f778076 Compare February 21, 2017 17:37
@kevina
Copy link
Contributor Author

kevina commented Feb 21, 2017

@Kubuxu I fixed my comments to include the start with the name of the function

@whyrusleeping I corrected the typos you pointed out.

@whyrusleeping whyrusleeping added RFM and removed status/in-progress In progress labels Feb 22, 2017
@whyrusleeping
Copy link
Member

Alright cool, this is ready to go in 0.4.7

@whyrusleeping
Copy link
Member

@kevina got a merge conflict here. Mind rebasing?

For now it is always called with the helper function GetLinksDirect to
avoid any change in behaviour.

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina kevina force-pushed the kevina/enumerate-children-refactor branch from 9d57601 to e4bbd24 Compare March 2, 2017 10:23
@kevina
Copy link
Contributor Author

kevina commented Mar 2, 2017

@whyrusleeping sorry, missed this. Just rebased and things should be good to go.

@whyrusleeping
Copy link
Member

Thanks @kevina !

@whyrusleeping whyrusleeping merged commit 5ff552c into master Mar 2, 2017
@whyrusleeping whyrusleeping deleted the kevina/enumerate-children-refactor branch March 2, 2017 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants