ImproveSignature and comparison_coercion documentation
#13840
Conversation
github-actions
bot
removed
the
documentation
| /// since i32 and f32 can be cast to f64 | ||
| /// | ||
| /// For functions that take no arguments (e.g. `random()`) see [`TypeSignature::Nullary`]. | ||
| Coercible(Vec<TypeSignatureClass>), | ||
| /// One or more arguments that can be "compared" | ||
| /// One or more arguments cast to single, comparable type. |
There was a problem hiding this comment.
I think this is clearer and correct, though I would appreciate it if @jayzhan211 could double check
alamb
changed the title
Signature and comparison_coercion documentation
| /// One or more arguments cast to single, comparable type. | ||
| /// | ||
| /// Each argument will be coerced to a single type using the | ||
| /// coercion rules described in [`comparison_coercion_numeric`]. |
There was a problem hiding this comment.
Why numeric? Strings are also comparable
There was a problem hiding this comment.
yes, but we have numeric currently, update doc when code is updated to be consistent
There was a problem hiding this comment.
I agree -- this code could be improved / named better. I looked at the implementation and there are quite a few differences between comparison_coercion_numeric and comparison_coercion that I didn't understand and thus didn't attempt to document
It would be great if someone could
| /// - For functions that take no arguments (e.g. `random()` see [`TypeSignature::Nullary`]). | ||
| /// - If all arguments have type [`DataType::Null`], they are coerced to `Utf8` |
There was a problem hiding this comment.
I am just documenting what I think the code does 🤷 -- I agree this area of the code could be improved
There was a problem hiding this comment.
To fix this, we might need to find a way to differentiate string literal as unknown type v.s. string type. Not yet have time to investigate on this.
There was a problem hiding this comment.
In some magic ideal world where I had infinite time, I have dreamed about refactor the coercion code into a set of structs that encoded the rules in a more structured / composable manner. But I don't have time to pursue that dream at this time 🏃
Co-authored-by: Piotr Findeisen <[email protected]>
|
Thanks again @findepi and @jayzhan211 |
Which issue does this PR close?
TypeSignature::NullAry-->TypeSignature::Nullaryand improve comments #13817Rationale for this change
While improving the docs in #13817, @findepi identified additional areas that were not super clear
What changes are included in this PR?
Try to improve the documentation about coercion more.
Are these changes tested?
By doc CI tests
Are there any user-facing changes?
Better docs, no functional change