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 path/family sibling traversal methods #5230

Merged
merged 1 commit into from Feb 7, 2017
Merged

Conversation

chitchu
Copy link
Contributor

@chitchu chitchu commented Jan 28, 2017

Q A
Patch: Bug Fix? N
Major: Breaking Change? N
Minor: New Feature? Y (QoL)
Deprecations? N
Spec Compliancy?
Tests Added/Pass? Y
Fixed Tickets Fixes #5223
License MIT
Doc PR
Dependency Changes

Adds path/family sibling traversal methods

@mention-bot
Copy link

@chitchu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo and @boopathi to be potential reviewers.

@codecov-io
Copy link

codecov-io commented Jan 28, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@283d9cb).

@@            Coverage Diff            @@
##             master    #5230   +/-   ##
=========================================
  Coverage          ?   89.25%           
=========================================
  Files             ?      203           
  Lines             ?     9862           
  Branches          ?     2624           
=========================================
  Hits              ?     8802           
  Misses            ?     1060           
  Partials          ?        0
Impacted Files Coverage Δ
packages/babel-traverse/src/path/family.js 87.06% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 283d9cb...e927482. Read the comment docs.

@existentialism existentialism added pkg: traverse PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jan 28, 2017
Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

What happens if there is no prevSibling or nextSibling?

}

export function forEachNextSibling(callback = Function.prototype) {
let _key = this.key, nextSibling = this.getSibling(++_key);
Copy link
Member

Choose a reason for hiding this comment

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

To improve readability, could you split theses two lines?

To be consistent in the coding style, _key could be then a const.

}
}

export function forEachPrevSibling(callback = Function.prototype) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the callback to be undefined and add a check before calling it.

return this.getSibling(this.key + 1);
}

export function forEachNextSibling(callback = Function.prototype) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just like Array.prototype.forEach allows a context argument, it may be worth including that second argument here too.

Copy link
Member

Choose a reason for hiding this comment

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

@chitchu Do you have a specific usecase in mind for this? If we add one like this, why not .map or .filter or any number of other array methods? I'd probably be more in favor of a getAllNextSiblings that returned an array, rather than re-implementing array-like behavior.

Copy link
Member

Choose a reason for hiding this comment

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

The original issue is #5223 by @kangax if you didn't see it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed the method in favor of getAllNextSiblings and getAllPrevSiblings which returns an array. But also accepts a callback and a thisArgs parameter. Once a callback parameter has been provided this will behave similar to Array.prototype.map.

@chitchu
Copy link
Contributor Author

chitchu commented Jan 29, 2017

What happens if there is no prevSibling or nextSibling?

@xtuc It would return a NodePath object but with a node: undefined property. This looked to me like a design choice and didn't want to alter how it behaved.

});

it("should return traverse sibling nodes", function () {
assert.strictEqual(!!sibling.getNextSibling().node, true, "has property node");
Copy link
Member

Choose a reason for hiding this comment

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

Can just do assert.ok(sibling.getNextSibling().node) here and further down.

let siblings = [];
const hasCallback = callback && typeof callback === "function";
while (sibling.node) {
siblings = siblings.concat(hasCallback ? callback.call(thisArg, sibling) : sibling);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just push to array? Overwriting and concating array every time is almost definitely slower.

return siblings;
}

export function getAllPrevSibling(callback, thisArg = this) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this kind of API, where we mix callback and return. getAllPrevSibling sounds weird (vs. getAllPrevSiblings). Why not just getAllPrevSiblings to return all siblings, which could then be iterated over with getAllPrevSiblings.forEach(sibling => ...)

for (const name in nonHoistedExportNames) {
hoistedExportsNode = buildExportsAssignment(t.identifier(name), hoistedExportsNode).expression;
}
// avoid creating too long of export assignment to prevent stack overflow
Copy link
Member

Choose a reason for hiding this comment

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

Something went wrong with rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh dear me, indeed it got messed up.

@chitchu chitchu force-pushed the master branch 3 times, most recently from 00392df to 3a42557 Compare January 31, 2017 01:39
@chitchu
Copy link
Contributor Author

chitchu commented Jan 31, 2017

@kangax All requested changes have been completed.

@kangax
Copy link
Member

kangax commented Feb 1, 2017

LGTM. @hzoo @boopathi anything from you?


it("should return all preceding and succeeding sibling nodes", function () {
assert.ok(sibling.getAllNextSiblings().length, "Has next sibling");
assert.ok(sibling.getNextSibling().getAllPrevSiblings().length, "Has prev sibling");
Copy link
Member

@hzoo hzoo Feb 1, 2017

Choose a reason for hiding this comment

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

Do we want some assert.equal tests here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Need to make sure the method returns the correct number of siblings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hzoo Added two tests against getAll(Next,Prev)Siblings().length

@hzoo
Copy link
Member

hzoo commented Feb 1, 2017

looks good?

@xtuc
Copy link
Member

xtuc commented Feb 1, 2017

LGTM 👍

I think we're missing Flow annotations. @chitchu would you mind adding some on the functions you created?

* getPrevSibling
* getNextSibling
* getAllNextSiblings
* getAllPrevSiblings
@chitchu
Copy link
Contributor Author

chitchu commented Feb 1, 2017

@xtuc Done! I'm not too familiar with Flow type, so I'm not quite sure if I've done them correctly. 😅

@kangax kangax merged commit 1ba4a3f into babel:master Feb 7, 2017
existentialism pushed a commit that referenced this pull request May 19, 2017
* getPrevSibling
* getNextSibling
* getAllNextSiblings
* getAllPrevSiblings
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: traverse PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add path.getPrevSibling, path.getNextSibling, etc.
9 participants