Skip to content

Update api.md#287

Open
buom wants to merge 1 commit intoschibsted:masterfrom
buom:patch-1
Open

Update api.md#287
buom wants to merge 1 commit intoschibsted:masterfrom
buom:patch-1

Conversation

@buom
Copy link
Copy Markdown

@buom buom commented Mar 8, 2023

correct the default filter

correct the default filter
Copy link
Copy Markdown
Collaborator

@larsga larsga left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the wrong method call in the documentation. That's much appreciated! 👍

Unfortunately, the other suggested change is wrong (see detail comment). If you can remove that I'll merge the PR.

Comment thread docs/api.md

```
. == null or . == {} or . == []
. == null or . == {} or . == [] or not(.) == false
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This addition is not correct. As you can see here it really is null, {}, and [] that are the omitted values.

The suggested change would, for example, filter out true, and 5, and so on.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@larsga Here we have two different implementations:

  1. Default implementation: NodeUtils.isValue(value); - here
  2. Custom implementation: NodeUtils.isTrue(jslt.apply(value)); - here

NodeUtils.isValue(value) != NodeUtils.isTrue(value)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, that's true, but the example expression says how to reproduce the default behaviour, which is your number 1.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the right fix would be to negate the expression, right?

. != null and . != {} and . != []

When the filter returns true, it means we want to keep the value. So here, we want to keep all values that are (1) not null AND (2) not {} AND (3) not [].

@larsga larsga self-assigned this Mar 8, 2023
@larsga larsga added the bug Something isn't working label Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants