-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 WebserverAPI for obtaing tags as json #4558
base: master
Are you sure you want to change the base?
Conversation
This commit creates a new route `/recipes/default/tags.json` (boldly copying from `/recipes/default/tiddlers.json`). It returns both tags for which a tiddler exists (i.e. `"tiddlerExists": true`) and tags for which no tiddler exists (i.e. "tiddlerExists": false`). Furthermore, one can use the queryParameter `count_tagged_tiddlers` to get the count of tiddlers having that particular tag. However, the default is not getting the count for performance concerns. To allow for the internal qury used for `/recipes/default/tags.json` (i.e. `[tags[]!is[system]sort[title]]`) a new config tiddler was added. Example default: ``` GET http://127.0.0.1:8080/recipes/default/tags.json ``` ```json [ { "title": "baretag", "tiddlerExists": false }, { "title": "colortag", "color": "#eda9a9", "created": "20200413104646112", "modified": "20200413104647214", "type": "text/vnd.tiddlywiki", "revision": 0, "tiddlerExists": true }, { "title": "icontag", "created": "20200413104652331", "icon": "$:/core/images/add-comment", "modified": "20200413104652353", "type": "text/vnd.tiddlywiki", "revision": 0, "tiddlerExists": true }, { "title": "tiddlertag", "created": "20200413104659294", "modified": "20200413104710342", "type": "text/vnd.tiddlywiki", "revision": 0, "tiddlerExists": true } ] ``` Example queryParameters: ``` GET http://127.0.0.1:8080/recipes/default/tags.json Parameters: filter = "[tags[]!is[system]sort[title]]" # this is the only query allowed by default. count_tagged_tiddlers = "true" # count usages of tag exclude = "," # prevent exclusion of textfield ``` ```json [ { "title": "baretag", "tiddlerExists": false, "taggedCount": 1 }, { "title": "colortag", "color": "#eda9a9", "created": "20200413104646112", "modified": "20200413104647214", "type": "text/vnd.tiddlywiki", "revision": 0, "tiddlerExists": true, "taggedCount": 1 }, { "title": "icontag", "created": "20200413104652331", "icon": "$:/core/images/add-comment", "modified": "20200413104652353", "type": "text/vnd.tiddlywiki", "revision": 0, "tiddlerExists": true, "taggedCount": 1 }, { "title": "tiddlertag", "created": "20200413104659294", "modified": "20200413104710342", "type": "text/vnd.tiddlywiki", "text": "this is a tag which is described by a tiddler", "revision": 0, "tiddlerExists": true, "taggedCount": 1 } ] ```
Thanks @idotobi, and welcome to the project. One minor thing is to ask if you could please sign the Contributor License Agreement, following the instructions in contributing.md in the root of this repo. A couple of thoughts on this PR:
|
I just sent out the CLA signature PR (see #4581). |
@Jermolene:
Yes, this is doable and this is actually what I did in the first place. The problem with this solution is that it does not give you tags which are not at the same time tiddlers. The problem here is Is there a better way to not only get tiddlers but also 'bare'-tags? If this is possible with existing APIs that would be great.
I totally understand that you want to keep the API as small as possible. I was tinkering with the idea of easily creating reference tiddlers for the current website (similar to bookmarks) with a firefox extension. For this it would have been handy to be able to get the tags for auto-completion. However, my project does not necessarily depend on this feature, so there is no need to go out of the way to implement it. I just thought it might be a useful route to have 😇 . Just for my future reference can something like a route be also implemented as a plugin? |
Hi @idotobi that makes perfect sense. Perhaps the way to go might be to add a new query string parameter "include-missing=yes" that would cause missing tiddlers to be also returned? We could return them as a tiddler consisting of just the title? |
@Jermolene : That sounds like a good idea. I would propose to call the query string parameter On the expected response: {
"title": "tiddlertag",
"created": "20200413104659294",
"modified": "20200413104710342",
"type": "text/vnd.tiddlywiki",
"text": "this is a tag which is described by a tiddler",
"revision": 0,
} What should be return for non-tiddler-tags? Do you mean a simple json object like the following: {
"title": "non-tiddler-tag"
} This way it would be easy to iterate over it and by testing whether the json-object has a One could also give it an explicit type, but I'm not sure what the correct type for non-tiddlers should be. {
"title": "non-tiddler-tag",
"type": ???
} |
The only field, that is important for a tiddler is "title". So you are good. If no type is used, it will default to "text/vnd.tiddlywiki" |
Ok, so @pmario you would propose to represent non-tiddlers as the following? {
"title": "non-tiddler-tag"
} |
hmmmm, ... yes. BUT You are not allowed to create them as real tiddlers. In reality they don't exist. You only get them back because you explicitly requested: "include-missing=yes" |
@pmario : Correct. I'm just trying to figure out how to properly represent non-tiddlers in the As I see it, whatever the response would be it should be easy to parse for the downstream application using it. The two important criteria I see for that purpose are:
|
In the OP there has been a |
Move the functionality to include non-tiddlers (useful for obtaining all tags) from the previously new created route `/recipes/default/tags.json` to the already existing route `/recipes/default/tiddlers.json`.
I updated the pull request. Following request:
will yield: [
{
"title": "baretag",
"non_tiddler": true
},
{
"title": "colortag",
"color": "#eda9a9",
"created": "20200413104646112",
"modified": "20200413104647214",
"type": "text/vnd.tiddlywiki",
"revision": 0
},
{
"title": "icontag",
"created": "20200413104652331",
"icon": "$:/core/images/add-comment",
"modified": "20200413104652353",
"type": "text/vnd.tiddlywiki",
"revision": 0
},
{
"title": "tiddlertag",
"created": "20200413104659294",
"modified": "20200413104710342",
"type": "text/vnd.tiddlywiki",
"revision": 0
}
] @pmario and @Jermolene : What do you think about that approach? |
It is in fact legal in TW5 to have a tiddler with a title but no other field (try executing
Care would have to be taken to distinguish the case where "non_tiddler" is set to a string, which would mean that it was a real tiddler with a field of that name. I think the terminology could be improved. Perhaps the query parameter should be I've some other minor comments on the code, too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @idotobi!
@@ -13,13 +13,15 @@ GET /recipes/default/tiddlers/tiddlers.json?filter=<filter> | |||
"use strict"; | |||
|
|||
var DEFAULT_FILTER = "[all[tiddlers]!is[system]sort[title]]"; | |||
var DEFAULT_INCLUDE_NON_TIDDLERS = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for pulling this out as a constant? We tend to do this for readability (e.g. making the default tiddler readable and obvious) but here there's not much benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially together with your comment below you're right the apparent flexibility that comes from representing it as a default fallback parameter could cause more harm then good.
My intention was to improve readability and be consistent with the DEFAULT_FILTER, but I see they are actually semantically different from one another.
|
||
exports.method = "GET"; | ||
|
||
exports.path = /^\/recipes\/default\/tiddlers.json$/; | ||
|
||
exports.handler = function(request,response,state) { | ||
var filter = state.queryParameters.filter || DEFAULT_FILTER; | ||
var include_non_tiddlers = state.queryParameters.include_non_tiddlers || DEFAULT_INCLUDE_NON_TIDDLERS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely we need to test that state.queryParameters.include_non_tiddlers is set to "yes"?
This is dangerous because if DEFAULT_INCLUDE_NON_TIDDLERS were true and state.queryParameters.include_non_tiddlers were false then the setting would end up being true. But as I say, I don't see the value in having a default fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. If the line were state.queryParameters.include_non_tiddlers || false
would this already resolve your concern, or do think it's still better to require yes
as the positive value for the query parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test explicitly for the positive value "yes".
response_body.push(tiddlerFields); | ||
} else { | ||
if(include_non_tiddlers) { | ||
let entry = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We stick to ECMAScript 2015 to keep compatibility with older JS environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that let
should be var
? I just started to learn javascript so I'm not yet solid in my distinction between the different standards.
I also tried to seach for ECMAScript 2015 and it seems to support let
(https://www.ecma-international.org/ecma-262/6.0/#sec-let-and-const-declarations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm .. ES2015 is ES6 which would allow let
and arrow functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I meant to say ES5.1
tiddlers.push(tiddlerFields); | ||
response_body.push(tiddlerFields); | ||
} else { | ||
if(include_non_tiddlers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would generally use "includeNonTiddlers" in preference to underscores. But with the changes I suggested the variable should be "includeMissing".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you also prefer the camelCase in the json response?
The only problem I'd see with "includeMissing"is that it sounds a little bit paradox at first sight. How can one include something that is missing? That being said I don't have strong feelings on "includeMissing" vs. "includeNonTiddlers" and since you are definitely the one to judge what's intuitive for the TiddlyWiki community I'll go with whatever you prefer. Just let me know and I'll implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you also prefer the camelCase in the json response?
I think the JSON response should also be includeMissing: true
The only problem I'd see with "includeMissing"is that it sounds a little bit paradox at first sight
Yes quite! I think it's consistent, though.
@@ -0,0 +1,2 @@ | |||
title: $:/config/Server/ExternalFilters/[tags[]!is[system]sort[title]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the core should be ship with too many external filters enabled, especially ones that aren't used by the core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Would there be a good spot in the documentation were I could include a section on how people can use the "new" extended server API? Otherwise, I'd expect it to stay a hidden feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a good spot in the documentation were I could include a section on how people can use the "new" extended server API?
Yes we should update the existing "WebServer API: Get All Tiddlers" docs tiddler.
@Jermolene : It turned out that I didn't need this feature for my project so I didn't spend much time on this lately. If you still think this is a valuable addition I could finish the PR, otherwise I would just close it. What would you prefer? |
Nothing to apologize :) I'll rebase the PR then and take in your latest comments to make it ready to merge (probably this weekend). |
Hi @idotobi we're at a good point to work on this PR if you're still available? It looks like there's a merge conflict to be resolved, and I think some or all of the code comments still need resolution. |
This commit creates a new route
/recipes/default/tags.json
(boldly copying from/recipes/default/tiddlers.json
).It returns both tags for which a tiddler exists (i.e.
"tiddlerExists": true
) and tags for which no tiddler exists (i.e. "tiddlerExists": false). Furthermore, one can use the queryParameter
count_tagged_tiddlers` to get the count of tiddlers having that particular tag. However, the default is not getting the count for performance concerns.To allow for the internal qury used for
/recipes/default/tags.json
(i.e.[tags[]!is[system]sort[title]]
) a new config tiddler was added.Example default:
Example queryParameters: