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

consider changing Node.get to always assert #2878

Closed
ianstormtaylor opened this issue Jun 12, 2019 · 2 comments
Closed

consider changing Node.get to always assert #2878

ianstormtaylor opened this issue Jun 12, 2019 · 2 comments
Labels
Milestone

Comments

@ianstormtaylor
Copy link
Owner

Do you want to request a feature or report a bug?

Idea / improvement.

What's the current behavior?

Right now we have:

element.getNode(path)

This can return either a node or null if nothing is found at that path. And you can use element.assertNode(path) instead when you want to be sure of retrieving a node at the path. But this is awkward because it's easy to forget.

What's the expected behavior?

In the future with #2495 we'll have:

Node.get(path)

I think it might make sense to make this one throw if it doesn't find something. And potentially introduce an alternative Node.maybe(path) which can also return null. This way errors in path-based logic are more clear. And when you really want to check a path that might not exist it's more obvious.

I find that most of the time I'd like the assertion protection.

@nz-chris
Copy link

nz-chris commented Jun 16, 2019

I'm unsure about parts of this, but the one thing I am sure about is that I don't like the name maybe as a function of a Node. More broadly, my preference would be:
element.getNode(path) will throw an error if there is no node at the path, but element.get(path) will just return whatever (if anything) is at the path (including null). These names make sense to me as getNode sounds more precise, whereas get is just 'give me whatever may be there'.

@ianstormtaylor ianstormtaylor added this to the Remove Keys milestone Aug 31, 2019
@ianstormtaylor ianstormtaylor mentioned this issue Nov 6, 2019
@ianstormtaylor
Copy link
Owner Author

Fixed by #3093.

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 a pull request may close this issue.

2 participants