[Accepted] SDL 0004 - SendLocation RPC Updates about sdl_evolution HOT 9 CLOSED

I have a few questions / concerns before posting a review on this proposal as a whole:

1. The SendLocation request has a parameter stating: <param name="deliveryMode" type="DeliveryMode" defvalue="PROMPT" mandatory="false">. This says it is not mandatory, but has a default value. Should the mobile library always send the default value if the developer does not specify, or should it exclude this parameter to have the default value used on Core's side?
2. The SendLocation request has a parameter stating:

<param name="timeStamp" type="DateTime" mandatory="false">
  <description>
      timestamp in ISO 8601 format
  </description>
</param>

But a "DateTime" is not the same thing as an ISO 8601 Timestamp, which looks like: 2016-08-17T20:08:34.263Z.

1. I would prefer to see exactly what's new, for example, the DateTime struct is new, and we should be careful to make this fully reusable in the future.
2. Some descriptions contain text such as: <description>Hypernym for e.g. district</description>, but "e.g." literally means "for example", so this means "for for example", which is nonesense.
3. What was the reasoning for each of the maxLengths on the parameters of the OASISAddress struct? It seems to me that for maximum flexibility, they should just be arbitrarily large rather than arbitrarily small.

EDIT: Thanks markdown for the crappy numbered list...

from sdl_evolution.

A

1. Core will apply the default value.
2. The intent is to pass the date-time information, but I can see how labeling it as ISO 8601 format could be a little misleading. This can be edited. The idea would still be to capture the necessary elements of time.
   B
3. Do you think anything additional is required for it to be reusable in the future ?
4. Do you mean, it should "Hypernym for district" instead? If so, will edit it.
5. The range should be sufficiently large enough to accommodate the data. The idea is that the complete address is split into the corresponding parameters. If you still think is too restrictive, the range can be expanded. Do you have any recommendations?

from sdl_evolution.

1. Wouldn't it be better for us to simply pass an ISO8601 timestamp string instead of building a whole DateTime struct?
2. Yeah "hypernym for district" is more correct.
3. I just don't see why we're limiting the length in that way, it would seem to me better to simply allow 500 characters or so on any field.

from sdl_evolution.

1. I think having it as struct makes it easier to handle on the core side. More over it is also similar to the existing way of using the media clock's StartTime.
2. Ok
3. I guess either ways it does not impact much. We can make it as 500 as well - although maybe for countrycode and postalcode we keep the current values

from sdl_evolution.

1. That's fine, it's just a lot more data over the wire. A few more suggestions for it, then, would be to change "wrt" to say "with regard to", and to add a millisecond field. Second, would it make sense to have all parameters optional, so that the DateTime struct can be used to specify a date or a time or both?
2. I'm not sure why. It doesn't hurt to raise it, and it just prevents us discovering a possible edge case where we end up needing to alter the RPC spec to raise the limit.

from sdl_evolution.

Review

Is the problem being addressed significant enough to warrant a change to SDL?

Yes, this problem makes sense to fix in SDL. Built in navigation systems should have the ability to go directly to an address and not just a point.

Does this proposal fit well with the feel and direction of SDL?

Yes.

If you have used competitors with a similar feature, how do you feel that this proposal compares to those?

N/A

How much effort did you put into your review? A glance, a quick reading, or an in-depth study? Please state explicitly whether you believe that the proposal should be accepted into SDL.

I did a fairly deep study on the components of this review. I believe this proposal should be accepted into SDL, but only with modifications that I have discussed above.

from sdl_evolution.

• I agree that all the parameters of the DateTime struct should not be mandatory, so that the reusability of the struct increases.
• I would agree that adding a millisecond field would be ideal so that we future-proof the struct.
• Increasing the size of all the values in OASIS would allow us be sure we don't run into any edge cases (hopefully).

Other than these comments, LGTM 👍

from sdl_evolution.

This proposal is accepted contingent on changes being made in an additional pull request:

• DateTime struct parameters are all optional.
• DateTime struct should include a millisecond field.
• OASIS parameter size values should be uniformly increased.
• "wrt" should be changed to "with regard to"
• Update comments referring to ISO8601 timestamp to refer to DateTime struct
• Update comments to say "hypernym for ____"

from sdl_evolution.

Issues entered:
Android
Core
iOS

from sdl_evolution.