-
Notifications
You must be signed in to change notification settings - Fork 46
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
Encode int as string #1179
Comments
Hey @lmazuel , in the past, we encountered similar issue where a property was defined as a string, but its value was in an integer format. We requested the service team to correct the Swagger spec, as this was identified as a definition bug. Why are we now considering this a feature for TypeSpec? |
one use case is representing |
Below discussion is on the condition that emitter is to support a general feature. Please let dev know whether we have such requirement, besides that Batch bug (where backend should send an int, but instead send a string and cannot fix that due to backward-compatibility requirement). For the Batch bug, I'd recommend Batch service write The full typespec would be
Question:
A few proposals, of different scope of support
PS: by "basic scalar", I include "numeric types", "string", "boolean" from built-in-types |
With above assumption, JS decided to use number to represent So talking about encode with int epic, do we have similar scenario where we really care about precision or some other reasons? Actually we have lots of definition for Back to weidong's proposal, I would prefer the option 1 with only allowing encoding this with necessary types e.g int64 or decimal. I haven't see any values with case |
Yeah this encoding would only be allowed to target string(in the same way that Additionally same comment as before I don't think |
@weidongxu-microsoft you're on point that this is Agree with you all, let's keep things simple and accept only the scenario string/int for now, there is no need to get complicated. And if someone one day comes with another conversion scenario, we can investigate at time the validity of that scenario. |
@timotheeguerin are you suggesting I kind of get this because Is there a list of |
I understood @timotheeguerin as: @encode("decimal", string)
foo: int32 would convert @encode("oct", string)
foo: int32 would convert IOW, still use the string parameter to describe the format you want it to look like, and usually we use base 10 (or decimal) |
Might be confusing to use |
No not decimal as floating point decimal as decimal notation of numbers(how you write numbers normally) But yes that was my point that |
Could maybe be
Also on a related note with encoding we are thinking that using string values it not great and it is better to keep the enum values as the only way for new ones So could have that(name tbd with above discussion as well) enum NumericEncoding {
decimal,
hexdecimal,
octal,
binary,
} |
@lmazuel except int not sure we could scope this a little to consider the |
Maybe we could limit this to integer (int32, int64, safeint) to string, for now. There could be some nuance of the "decimal" representation, but may not be too bad. If we allow float to string, there could be more questions, e.g. whether |
Missed the meeting notes here, problem isn't anymore about the THe current proposal is to have enum NumericEncoding {
base10,
// With option to add the following in the future
// base16,
// base 8
} That you can then use @encode(NumericEncoding.base10)
a: int64; The current question is
|
Proposed changed signature for decorator extern dec encode(
target: Scalar | ModelProperty,
encoding: string | EnumMember,
encodedAs?: Scalar
);
extern dec encode2(target: Scalar | ModelProperty, encodedAs: Scalar);
extern dec encode2(
target: Scalar | ModelProperty,
encodingOrEncodedAs: string | EnumMember | Scalar,
encodedAs?: Scalar
);
And then you can call @encode(string) val: int64 restrict this overload format to only numeric scalar types and |
IMHO, I'd avoid I think we should be clear about the conversion rule (it need not to be an RFC, but we should be explicit about what is a valid string representation, what is not. I am not aware that there is consistency between different languages, when convert string to/from a scalar. Say convert String to Boolean, when de-serialize the JSON). ,NET's Golang also accept |
We are limiting that to numeric types and it means the decimal representation of the number as if you were to serialize the number to a json string |
Yeah, that works. Or we may say that JSON numeric wrapped in quote is acceptable. |
Maybe irrelevant to the detailed design. But I'd like to clarify one thing - what does the typespec type stand for?
|
|
Thanks for the explaination and it makes sense considering we have guideline on no client-side validation. So can I understand typespec type stands for client type or the type we want to show to our customers? |
Not sure I see the distinction here The typespec type represent the logic type. In some client/server/other target you might not be able to represent it as that logical type so you might choose to keep it as the wire type(e.g. |
I think the logic type is what I mean here. One more question - with below example we would expect different logic type if emitter could somehow differentiate the utcDateTime or offsetDateTime, but on the wire they are the same format?
|
Yeah they both use |
Problemm:
We have a service team that requires string on the wire, but this is actually always an integer. Given the simplicity of converting string to int (and vice versa), we want to add support for declaring that conversion, and get emitters supporting it, to avoid manual code for service team cross-language.
Edit:
After discussion, it became clear that the
encode
parameter is an encoding, and therefore shouldn't bestring
, but something describing what the encoding will look like. In the usual case, we would want base-10, which would serialize12
as"12"
. As base-10 is technically "decimal", we could use that term, but it might be conflated with the decimal type. The current consensus seems to benumber
, as a fair semantic of base-10:Original proposal at creation:
TypeSpec
Spec
Implementation
The text was updated successfully, but these errors were encountered: