-
Notifications
You must be signed in to change notification settings - Fork 4
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
90%: Add computed fields #40
Conversation
break; | ||
default: | ||
$value = null; | ||
} |
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.
Arithmetic operators contains: "+", "-", "", "/", "%", "*"
Did we miss "**" from this switch?
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.
No - it should have been removed from the valid operators: exponents were introduced in PHP 5.6, so I don't think we should support them here.
Reading through this, I was thinking there is a lot of parsing code - is there some library in PHP that might do this for us? A PHP yacc/bison? I guess we are already parsing the tablespec like this, and this is just extending what we have. So - other than my question above on "**" - 👍 |
I was also concerned about the size of the validation code (which I think is what you mean about parsing), especially given that it needs to be done on every request. I sort of wonder about keeping them, but making the actual bit that runs on MongoTripod::loadConfig() much smaller and put a more comprehensive one as a script. |
The only downside to this is that if your tripod config is generated on the fly (like it is in TARL). |
My first thoughts on this is that we seem to have an element of "code is data, and data is code" hovering around here, and this is a problem which has already been solved by languages such as LISP. I have found a couple of sites where people are discussing embedding conditionals within JSON, like we are trying to do, and lisp-like s-expressions seem to be approaches which are favoured, as they directly translate to this problem. http://alandipert.tumblr.com/post/7193534410/discover-lisp-in-your-web-browser-with-javascript - part way down this page the author proposes a JSONscript language (I googled - it doesn't actually exist yet), but I wonder if it might provide some interesting ideas for us here. Perhaps we could write a simple parser for JSONscript and open source it. There is a stack overflow post here where the top answer is proposing a similar design: http://stackoverflow.com/questions/20737045/representing-logic-as-data-in-json Having said that, there is nothing inherently wrong with this approach if we are too far down the rabbit hole to change it now - I was just thinking in terms of standing on the shoulders of giants, and such. It would also help with the above thoughts on validation by separating out that code from tripod itself. |
@lordtatty Are you proposing that the "value" value for the fieldName "type" should look something like: ["_replace_",["bibo","","$mainType"]] and the "value" value for fieldName "referenceDiff" should look like: [
"_conditional_",
[
"if",
[
">",
["$referenceCount", "$referencedCount"]
],
"then",
[
"_arithmetic_",
[
"-",
["$referenceCount","$referencedCount"]
]
],
"else",
[
"_arithmetic_",
[
"-",
["$referencedCount","$referenceCount"]
]
]
]
] |
It's been a little while since I touched something like lisp myself, but for the latter I was thinking something along the lines of:
Maybe could make it easier a function like MAXMIN so we don't need to do two separate MAX and MIN calls, but in terms of raw functionality, something like that. |
I think all of the comments here have been addressed:
|
{ | ||
"fieldName": "type", | ||
"value": { | ||
"_replace_" : { // _replace_ is a function name |
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.
Should the underscores be removed?
@@ -296,6 +318,14 @@ protected function loadConfig(Array $config) | |||
{ | |||
throw new MongoTripodConfigException("View spec does not contain " . _ID_KEY); | |||
} | |||
if(!isset($spec['from']) || !in_array($spec['from'], $this->getPods($storeName))) |
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've got a validateTableSpec
function to validate table specs, but validation of view specs (and search specs) are done inline. Worth moving to a function to keep it consistent?
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.
Originally, I had done this, but, actually, the amount of validation done for search and view specs is the same as we do inline for table specs.
Really, it's just that we do a lot more validation for table specs.
👍 |
"then" and "else" values may be of any type, including another function. To replicate 'else if', you can use another | ||
conditional function as the value of 'else'. | ||
|
||
Arithmetic 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.
Do you want something in here to tell you what arithmetic functions are available?
"replace": { | ||
"search": "bibo:", | ||
"replace": "", | ||
"subject": "$x" |
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 noticed in the PR that uses this functionality that you have something like:
"subject": "$!concatenatedType"
- what does that do?
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 think that "!concatenatedType" is the name of the field - just confused me a bit as it looked like it could be an operator..
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 was based on an earlier version of this branch.
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.
READ THE COMMENTS, @rgubby!
looks good to me! 👍 |
…-data 90%: Add computed fields
This is a spike to throw up a proof of concept to allow generation of dynamic table values based on the values elsewhere in the generated table row document.
It also introduces the notion of ‘temp’ fields that aren’t stored in the collection: these fieldNames have a
"temporary": true
property.Example of a table spec with this
Example of document outputted above