The Wayback Machine - https://web.archive.org/web/20201128085329/https://github.com/nodegit/nodegit/issues/1588
Skip to content

/ nodegit

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

Jump to bottom

Need to call `walker.start()` for `tree.walk()` not documented except in example #1588

Open
foolip opened this issue Nov 20, 2018 · 2 comments
Open

Need to call `walker.start()` for `tree.walk()` not documented except in example #1588

foolip opened this issue Nov 20, 2018 · 2 comments
Labels
Good First Issue Help Wanted

Comments

@foolip
Copy link
Contributor

@foolip foolip commented Nov 20, 2018

System information

  • node version: v10.13.0
  • npm or yarn version: 6.4.1
  • OS/version/architecture: gLinux 64-bit (Google-internal, like Debian Testing)
  • Applicable nodegit version: v0.23.0 built from source

https://www.nodegit.org/api/tree/ has an example of how to use tree.walk(), but it doesn't mention a critical detail, which is mentioned in an example:

nodegit/examples/walk-tree.js

Lines 22 to 23 in 1f84d31

// Don't forget to call `start()`!
walker.start();

It looks like the start() method is added to the instance here:

nodegit/lib/tree.js

Lines 145 to 146 in 1f84d31

event.start = function() {
bfs(null, self);

It took me quite a while to work out what the problem was, looking at the EventEmitter documentation would not have helped. I'm not sure if subclassing from EventEmitter is a good idea, but having a TreeWalker class where the this is documented would have made it easier to discover.
@rigdern
Copy link

@rigdern rigdern commented Dec 1, 2018

I ran into this as well. I want to add a couple of specific points:

  • The doc comment claims that walk returns an EventEmitter but the returned object actually has an extra method (start) that EventEmitters don't have.
  • Similarly, the TypeScript type definition file indicates that walk returns an EventEmitter so you get a compiler error when attempting to call start.

Here are specific suggestions for improvement:

  • In the walk documentation, tell people to call start.
  • Create a new type called TreeWalkerEventEmitter which is just like EventEmitter but it has an additional start method. Include this type in the nodegit documentation and add this type to the TypeScript type definition file.
This was referenced May 23, 2019
Merged
Open
@billytrend
Copy link

@billytrend billytrend commented May 23, 2019

I have attempted to address the issues in the referenced PRs. Please let me know if you have any further suggestions for the changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Good First Issue Help Wanted
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@rigdern @foolip @implausible @billytrend
You can’t perform that action at this time.