Add some new functions to the standard library #217
Conversation
|
@lorenzosinisi whoa! thanks for taking the time to contribute this 🫶 ! Out of curiosity, how are you using this library currently? Would love to learn how it's being used, and hopefully you're finding it useful? |
|
Hi @smn thanks, and I hope the functions can be merged in the standard lib! I am currently using it to allow non-devs to write simple code to transform data, and yes it is useful! |
|
Hi @smn is there any chance to get this merged? |
lib/expression/callbacks/standard.ex
Outdated
| "Convert a date into any strftime format (ref: https://man7.org/linux/man-pages/man3/strftime.3.html)", | ||
| expression: ~s[text(datevalue(date(2022, 09, 14)), "%m/%d/%Y")], | ||
| result: "09/14/2022" | ||
| def text(ctx, value, format) do |
There was a problem hiding this comment.
How is this different from supplying the format directly to datevalue here? That function accepts a string formatting argument already:
expression/lib/expression/callbacks/standard.ex
Lines 119 to 149 in 81ea560
I can see the text() function name here being ambiguous and likely a source of confusion for folks given this seems like a generic function but is only really useful for date / datetime things.
That said, if you're wanting to use this in your own code, you could perhaps include it in a custom callbacks implementation ? The implementation isn't ideal but here's an example test case that does exactly that: https://github.com/turnhub/expression/blob/develop/test/expression_custom_callbacks_test.exs
There was a problem hiding this comment.
Reading https://support.microsoft.com/en-us/office/text-function-20d5ac4d-7b94-49fd-bb38-93d29371225c I understand why it's called text(), I guess it's a generic formatting function.
However, if we want to implement this we'll want to make sure to support other types of values as well. Until that is ready I would suggest keeping this one in a custom callbacks implementation?
There was a problem hiding this comment.
I would say we could remove this from this MR and leave it as custom callback, sounds good to me!
| "Remove the last word from a list of words, using spaces as separator between words ", | ||
| expression: "remove_last_word(\"foo bar\")", | ||
| result: "foo" | ||
| def remove_last_word(ctx, binary) do |
There was a problem hiding this comment.
this one's great as it complements the already existing remove_first_word()
| ~s[The SWITCH function evaluates one value (called the expression) against a list of values, and returns the result corresponding to the first matching value. If there is no match, an optional default value (the last one in the list if the list is odd) may be returned], | ||
| expression: ~s[SWITCH(5, 1, "Sunday", 2, "Monday", 3, "Tuesday", "No match")], | ||
| result: "No match" | ||
| def switch_vargs(ctx, arguments) do |
There was a problem hiding this comment.
thank you! This aligns with the excel function right? https://support.microsoft.com/en-us/office/switch-function-47ab33c0-28ce-4530-8a45-d532ec4aa25e
There was a problem hiding this comment.
yes indeed, matches "SWITCH" 👍
| "The ROUND function rounds a number to a specified number of digits. For example, if cell A1 contains 23.7825, and you want to round that value to zero decimal places you can do ROUND(23.7825)", | ||
| expression: "ROUND(23.7825)", | ||
| result: "24" | ||
| def round(ctx, value) do |
There was a problem hiding this comment.
same here, aligning with https://support.microsoft.com/en-us/office/round-function-c018c5d8-40fb-4053-90b1-b3e7f61a213c
| @expression_doc expression: ~s[MID("Fluid Flow", 20, 5)], | ||
| result: "" | ||
|
|
||
| def mid(ctx, text, start_num, num_chars) do |
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks so much for your contribution! It's highly appreciated. Before we merge this, can we discuss the implementation of text() ?
If you're unable to complete it then we can merge the others in the mean time and you can keep the text() implementation in a custom callback module.
If you're able to complete it then that's awesome, we'd love to see a complete implementation of text() merged.
It's up to you!
|
To simplify I have opted for removing (for now) the TEXT function, how does it look now? |
|
Given the CLA that's in place, are you happy for us to merge these and for them to fall under the Turn.io copyright as per the CLA.md document in the repo? It looks like the CLA github action is broken (I'm trying to fix it) but I also don't want to hold up this merging if you're happy with it. Give me a few hours to see if I can get this CLA to run again. Something with its permissions seems to have changed. |
|
ok it looks fixed, could you merge in the latest changes in the |
|
Sounds good to me yes! Thank you, and the same goes for the other PR have opened this morning 👌👌thank you!L
|
- MID - TEXT - SWITCH - REMOVE_LAST_WORD - ROUND The function MIX, TEXT, SWITCH and ROUND are done mirroring Excel functioality, instead REMOVE_LAST_WORD is done to mirror the existing REMOVE_FIRST_WORD
lorenzosinisi
force-pushed
the
add-some-more-functions
branch
from
1c61be1
to
6719360
Compare
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Hi @smn any chance of merging this one? |
|
thanks @lorenzosinisi ! Apologies for the delay in the review + merge, appreciate the contribution! |
I have added the following functions:
The function MIX, SWITCH and ROUND are done mirroring Excel functionality, instead REMOVE_LAST_WORD is done to mirror the existing REMOVE_FIRST_WORD
This should help adoption and code repetition over multiple projects using this library