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

extend get filter for data #1375

Closed
wants to merge 4 commits into from
Closed

Conversation

tobibeer
Copy link
Contributor

For a data tiddler, returns the value at the index specified in the operand

<$list filter="[<dataTiddler>get:data<dataIndex>]"/>

demo: http://tbdemo.tiddlyspot.com/#Index%20Filter

for a data tiddler, returns the value at the index specified in the
operand

```
<$list filter="[<dataTiddler>index<dataIndex>]"/>
```

do code-revision => first time user of the parser object
@tobibeer
Copy link
Contributor Author

Thinking about it, maybe it should be called value instead?

@aelocson
Copy link
Contributor

Thinking about it, maybe it should be called value instead?

Or data to clearly indicate the connection with data tiddlers. The word index denotes the name (or key) of the name–value pair. (I don't much like the word index myself, because it's too overloaded and thus it risks confusing people.)

do code-revision => first time user of the parser object

Actually, you don't need to use the parser object at all here. There are wrapper functions available, such as wiki.extractTiddlerDataItem and wiki.getTextReference.

However, this new operator is so reminiscent of the existing get operator that I suggest it would be better to combine the two, perhaps using get:data for the new option. The body of the anonymous function passed to source() in get.js would then be:

    if(tiddler) {
        var value = operator.suffix === "data" ?
            options.wiki.extractTiddlerDataItem(tiddler,operator.operand,"") :
            tiddler.getFieldString(operator.operand);
        if(value) {
            results.push(value);
        }
    }

@tobibeer
Copy link
Contributor Author

@aelocson, indeed. I very much like the idea of keeping the number of filters as small as possible, so using a suffix for get is indeed a much better approach. I'll do as you suggest, test, and actually change the code and ticket.

now you can do `[all[current]get:data[foo]]`
@tobibeer tobibeer changed the title index filter extend get filter for data Jan 23, 2015
@tobibeer
Copy link
Contributor Author

@Jermolene ...btw. welcome back. :-)

Although I am quite doubtful about the suggested performance increase, I went ahead and split the iterator in two, as suggested at the other filters.

However, there are two things I'm wondering about with get:

  1. It currently pushes duplicates into the result list when source has more than one tiddler. Is that advisable? Why / for what usecase? However, not doing so would have get closely resemble each.

  2. Is it actually correct for the current get filter to use getFieldString and thus ignore blanks over undefined? I don't think it is. After all, empty string is a possible value. What's more, it doesn't add a blank result to the output which thus shifts the list. If we do use getFieldString and allow non-blank duplicates anyhow, then why not always add an empty string, rather than not add at all?

@Jermolene
Copy link
Member

Hi @tobibeer

It currently pushes duplicates into the result list when source has more than one tiddler. Is that advisable? Why / for what usecase?

If you're reading a bunch of values and you want to be able to refer to a specific one, then we need to have the full array. If identical entries in the array were merged then the indices would no longer match.

Is it actually correct for the current get filter to use getFieldString and thus ignore blanks over undefined? I don't think it is. After all, empty string is a possible value

Intentionally we don't distinguish between a missing field and a blank field. The reason is that we don't have a value to use for missing fields.

@Jermolene
Copy link
Member

I think it would be more consistent with the other data index-related filter operators to introduce a new filter for this, and not reuse get.

@tobibeer
Copy link
Contributor Author

If you're reading a bunch of values and you want to be able to refer to a specific one, then we need to have the full array. If identical entries in the array were merged then the indices would no longer match.

That is what I was trying to say: This is not what we get. We don't get the full array. We only get values for those where the field is defined and not empty, so the result-set does not match the source set as it is right now. If we return duplicates, should we not also return blanks?

Intentionally we don't distinguish between a missing field and a blank field. The reason is that we don't have a value to use for missing fields.

What's wrong with ""? I know there's no tiddler corresponding to "", but whichever widget receives that entry in a result array from a filter ...should gracefully handle the case, imho.

I think it would be more consistent with the other data index-related filter operators to introduce a new filter for this, and not reuse get.

Would you perhaps prefer [data[index]] ...and have that suffix left for something else?

Currently, with text-references we are also fetching either fields or data. So, it seemed consistent to have the get filter do that as well, even though not making use of the text-reference syntax, surely.

@Jermolene
Copy link
Member

If we return duplicates, should we not also return blanks?

Ah, I see, yes, that does sound reasonable.

What's wrong with ""

Using "" to indicate a missing field is fine, it just isn't distinguishable from a field with the empty string as its value.

Would you perhaps prefer [data[index]] ...and have that suffix left for something else?

Well, I'd prefer to use the word "index" so that it's grouped with the other index-related filters.

So, it seemed consistent to have the get filter do that as well, even though not making use of the text-reference syntax, surely.

I agree, it was worth exploring.

@tobibeer
Copy link
Contributor Author

Using "" to indicate a missing field is fine, it just isn't distinguishable from a field with the empty string as its value.

Not as a filter result, that's true. However, I would expect "" to be part of a filter result, but not <undefined>.

Well, I'd prefer to use the word "index" so that it's grouped with the other index-related filters.

Haha, that's what this pull request started out as. ^_^

Ok, so I will leave the get filter with its inconsistency untouched for now.

Do you think that this index filter is indeed a worthy / needed addition for the core or would you, in fact, wait until someone asks for it?

@Jermolene
Copy link
Member

How about getindex as the operator name?

@aelocson
Copy link
Contributor

getindex would be admirably clear.

@tobibeer
Copy link
Contributor Author

getIndex sounds good, bundling it with get, namewise, while also having a relation to index.

Ok, so I revert to the initial request, change the name, add tests... and perhaps make a new pull request altogether, so that this thread remains as is for history's sake.

Just realized, I never updated that demo ^^.

@Jermolene
Copy link
Member

Great thanks @tobibeer

@tobibeer tobibeer closed this Jan 28, 2015
@tobibeer tobibeer deleted the index-filter branch January 28, 2015 20:33
@tobibeer
Copy link
Contributor Author

@Jermolene (& @aelocson) , some more questions...

If the source list had multiple data tiddlers with the same index, what do you expect the output to be: unique or duplicate values? I tend to think unique, to create a set. What would we gain from a list with duplicates? Same with get. Whom are we helping with duplicates if most all entries in source may not have that field or that index? If we allow duplicates, should there be a "once" filter to remove the duplicates later?

@aelocson
Copy link
Contributor

If there's a chance that any future operators may output duplicates, a once filter would be a sensible plan. If get is the only one there's ever likely to be, maybe a get:once option would be better,

@Jermolene
Copy link
Member

Hmm I wonder if one underlying problem here isn't that $tw.utils.stringifyList(["foo","","bar"]) should return foo [[]] bar and not foo bar.

@Jermolene
Copy link
Member

a once filter would be a sensible plan

Maybe dedupe?

@tobibeer
Copy link
Contributor Author

$tw.utils.stringifyList(["foo","","bar"]) => foo [[]] bar and not foo bar.

Sounds sensible. Does that have the potential to break anything?

@Jermolene
Copy link
Member

Does that have the potential to break anything?

The behaviour of $tw.utils.stringifyList an implementation error, I think; the function was originally used for tiddler titles and tags which are never blank. The current behaviour is not documented.

@aelocson
Copy link
Contributor

I agree about $tw.utils.stringifyList.

dedupe is OK, but perhaps distinct would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants