Skip to content
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

Choice-VR optional #180

Merged
merged 2 commits into from
May 24, 2017

Conversation

ghost
Copy link

@ghost ghost commented May 15, 2017

This proposal is about changing Choice.vrCommands to be not mandatory.

@ghost
Copy link
Author

ghost commented May 15, 2017

@theresalech The proposal is ready for review.

Copy link
Contributor

@theresalech theresalech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kshala-ford - can you please make the two minor updates to the "Proposed solution" section? Once those are addressed, I'll mark this proposal as "review ready" - thank you!

- `resultCode` = `INVALID_DATA`,
- `info` = "Some choices don't contain VR commands."

A planned proposal for a choice set manager could benefit from this proposal by automatically omit `vrCommands` if `vrCapabilities` is empty (assuming it already exist and the app is using it...).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change "omit" to "omitting" and "exist" to "exists" in this sentence.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


## Proposed solution

This proposal is to make the paramter `vrCommands` optional. This would allow apps to avoid unnecessary vr commands if they want to perform an interaction in manual mode. On the other hand it saves a lot of time of grammar computing on the head unit side.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"paramter" -> "parameter"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

## Impact on existing code

- This proposal is a major change on Core and HMI.
- It's a very minor change for the SDKs. Properties have to become optional/nullable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "major change", as in a major API change that requires a new major version of Core?

On the "SDKs", by which I assume you mean the mobile libraries, "a very minor change" is not precise. This is a minor version change (e.g. 1.X.0, where X changes).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not talking about version changes. I was comparing the effort to implement this proposal but I can remove those lines as they are pretty much pointless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify both the implementation effort and semantic version changes? For example, "This proposal will require a large effort to implement for Core and HMI, and will be a minor version change because..."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theresalech Although this proposal is not of a big deal I don't think I can decide about the version change and I don't believe it should be mentioned within the proposal (unless it's required to be careful when to add the feature).

I cannot estimate the effort on Core and HMI but I'm pretty sure it's more of an effort than on the library side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version changes are deterministic according to https://semver.org, so you absolutely should specify what the version change is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since when is it required to specify this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't required, but it's the first thing anyone will want to know. "What is the impact of the change on developers?" Which is answered by answering, "What is the type of version change?" So it isn't required, but it's certainly helpful.

@ghost
Copy link
Author

ghost commented May 17, 2017

@theresalech please mark this PR as ready as the requested change is not required.

@joeygrover
Copy link
Member

@kshala-ford even though it isn't "required" I would recommend you provide it.

@theresalech theresalech merged commit 6dbfeb6 into smartdevicelink:master May 24, 2017
@ghost ghost deleted the choice-vr-optional branch September 5, 2017 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants