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 nodes.parentNodes() and nodes.nonparentNodes(). #3195

Closed
wants to merge 1 commit into from

Conversation

maxkfranz
Copy link
Member

Ref: Add a method for nodes.nonparentNodes() to get the childless nodes #3182

Associated issues:

Notes re. the content of the pull request. Give context to reviewers or serve as a general record of the changes made. Add a screenshot or video to demonstrate your new feature, if possible.

  • Add nodes.parentNodes() : gets the nodes in the calling collection that are parents.
  • Add nodes.nonparentNodes() : gets the nodes in the calling collection that are NOT parents.
  • Add nodes.childlessNodes() alias : alias of nonParentnodes().
  • Add documentation.
  • Add tests.

Checklist

Author:

  • The proper base branch has been selected. New features go on unstable. Bug-fix patches can go on either unstable or master.
  • Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix. Check this box if tests are not pragmatically possible (e.g. rendering features could include screenshots or videos instead of automated tests).
  • The associated GitHub issues are included (above).
  • Notes have been included (above).

Reviewers:

  • All automated checks are passing (green check next to latest commit).
  • At least one reviewer has signed off on the pull request.
  • For bug fixes: Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

Ref: Add a method for nodes.nonparentNodes() to get the childless nodes #3182
@maxkfranz
Copy link
Member Author

@chrtannus @mikekucera

We discussed having convenient filter functions like this before. They'd be useful in EM, for instance.

The downside of these functions is that they could potentially be confused with other functions, like nodes.parents().

  • node.parent() : get the parent node of the calling node
  • node.ancestors() / node.parents() : get all ancestor nodes
  • (NEW) node.parentNodes() : get the nodes in the calling collection that are parents

See https://js.cytoscape.org/#collection/compound-nodes

What do you think? Let me know if you have ideas for clearer names.

@chrtannus
Copy link
Member

Yes, I find having nodes.parentNodes() in addition to node.parent(), but with different meanings, will be very confusing.
I agree we need better names. I'm thinking about it...

@chrtannus
Copy link
Member

How about nodes.filterParentNodes(), nodes.filterNonparentNodes(), etc.?

@maxkfranz
Copy link
Member Author

How about nodes.filterParentNodes(), nodes.filterNonparentNodes(), etc.?

Those are good ideas, though if we make the names long, you might as well just nodes.filter(n => n.isParent()) or nodes.filter(':parent').

I agree we need better names. I'm thinking about it...

I think the naming is important enough to hold off on merging this until we have good names, even if that means it goes in a later version next year.

An alternative to consider is putting more examples in the documentation for things like filtering to get the nodes that are parents.

@maxkfranz maxkfranz added the pinned A long-lived issue, such as a discussion label Dec 19, 2023
@maxkfranz
Copy link
Member Author

Let's consider more examples in the documentation for future. Closing.

@maxkfranz maxkfranz closed this Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned A long-lived issue, such as a discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants