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

Convert Vehicle Signal Client spec from markdown to respec #137

Merged
merged 4 commits into from
Mar 29, 2017

Conversation

pkinney
Copy link

@pkinney pkinney commented Feb 14, 2017

No description provided.

@Hirabayashi-KDDI
Copy link

Great! Thank you, Powell.
Let's publish FPWD of the client spec as soon as possible, replacing the client spec's name with Vehicle Information API Specification (VIAS) as decided in the Mini f2f of Automotive WG/BG at CES [1].
[1] https://www.w3.org/2017/01/05-auto-minutes.html

@pkinney
Copy link
Author

pkinney commented Feb 21, 2017

@Hirabayashi-KDDI: Thank you for the comment. I will make the update and push to this branch later today.

@pkinney
Copy link
Author

pkinney commented Feb 21, 2017

Updates made based on @Hirabayashi-KDDI's comments.

@aShinjiroUrata
Copy link
Contributor

aShinjiroUrata commented Feb 22, 2017

Hi Powell-san , thanks for the nice job!
Below are the things I noticed.
Please have a look.

A) 4. W3CClient() > Client Options > Connection Properties

In VISS, subprotocol is used('wvss1.0').
So, may be better to describe about subprotocol at here?

B) 4. W3CClient() > Client Options > Handler Properties > onError

The returned error is Error object defined in VISS?
If so, may be better to add 'refer VISS' 11.Errors for detail.' or something?

C) 4. W3CClient() > Functions

Looks some functions provide client and some are not.
Is there a rule?
(e.g. callback for get() doesn't provide client.
Is this on purpose?)

D) 4. W3CClient() > Functions: authenticate()

  • In VISS, word 'authorize' is used as method name.
    May be better to align to ’authorize’ or 'authenticate'?

According to below URL, in authentication, usuallly userID, password are used.
http://stackoverflow.com/questions/6556522/authentication-versus-authorization
So in this case, I guess, word authorize fits better?

  • The token written here is the same kind of token used in W3CClient constructor,
    which is described as authenticationTokens.
    Or one is authenticationToken and the other is authorizationToken?

  • In returned value, what value will be contained?
    Is it boolean to indicate success or failure?

E) 4. W3CClient() > Functions: get()

I suppose, callback should have value otherwise I don't see
how to use retrieved value.
Also, in VISS, timestamp value is also available when a value is retrieved.
I suppose, better to have timestamp available in VIAS as well.
e.g.

callback(err, res) { console.log("data="+ res.value); console.log("timestamp="+res.timestamp);}

F) 4. W3CClient() > Functions: subscribe()

In VISS, ranges, interval, minimum changes are handled in filters.
To make it aligned with VISS, the options should have only filter and onValue. Then interval, onChange should be specified inside filter.
What do you think?
If using same definition of filter, then VIAS library need to do nothing excpet just passing filter string to VISS' subscribe method.

G) 5. Subscription Object > EXAMPLE3

There is onExpire, I think, which is related to authentication's TLL.
Need to add description of onExpire?

H) 6. VSS Object > pathsByCSS()

How much portion of CSS selectors will be supported, do you think?
I guess supporting small part of CSS Selector is enough for this usage.
(Of course if many things are supported, it is a good new.)

In https://www.w3.org/TR/css3-selectors/#selectors
I guess these should be supported

E F : an F element descendant of an E element
E > F : an F element child of an E element

I'm not sure about these things. May be not very important..

E[foo="bar"] : an E element whose "foo" attribute value is exactly equal to "bar"
E:first-child : an E element, first child of its parent

Another point is how to describe about supported functions.
Just leave as implementation dependent is ok to me.

I) 6. VSS Object > EXAMPLE4

There are description of openWindow and openAllWindows.
I suppose typo.

J) General

I suppose better to add data type description in function definition (e.g. string , object, etc)

K) Others

In VISS, 'wss://' is MUST condition and in VIAS 'wss://' is written as strongly recommended.
Is this written differently on purpose?
I personally think 'strongly recommended' may be enough and MUST might be too strong since someone may want to use 'ws://' in a closed environment.

@Hirabayashi-KDDI
Copy link

I hasten to make another comment.
The primary purpose of VIAS is to define a high-level API to vehicle information, which is agnostic of the underlying vehicle information sources and architecture.

If the above policy applies to this spec, description about interaction between VISS and VIAS should be non-normative as a guideline for implementation. VIAS could be independent of VISS.

If you and everyone will be agreeable to the above thoughts, I will help editing works, referring to the previous spec [1].
[1]: https://www.w3.org/TR/vehicle-information-api/

@pkinney
Copy link
Author

pkinney commented Mar 7, 2017

@aShinjiroUrata: Thank you for your great comments. I have addressed them below individually.

A) In practice, this should be internal to the library. I don't believe that the abstraction we are providing should require this knowledge on the client side. Additionally, if this is the only option, then we probably shouldn't offer any way to change it (See my answer to point K).

B) It could be, but in addition to the errors returned by VISS there are client-side errors (i.e. timeout, no-connect, bad response format, etc.) that are also included here. We could make a note that the error format is left to the implementation and/or could require that when an error is returned directly by the VISS, that the structure/format be preserved here.

C) Hmm. I only provide a client to the callback on the connect function. I think I did that to provide for the change in state from a pre-connected client and a connected one. I don't think that that is done anywhere else and would rather just remove it from the callback.

D) "Authorize" is the appropriate term as as I am "authorizing" this connection for certain access that it didn't have before. The Authentication of the token I am using is up to the client's internal implementation since at this point I have already "Authenticated" myself with the token-issuing authority in order to get the token. If there is confusion, perhaps we can change the function name to applyToken or some such thing.

E) I would assume that the value property is actually the payload from the VISS server, which would include both the raw value and the timestamp, but will clarify that here.

F) Agreed, that we should directly reference the filter spec from VISS here if possible so as not to duplicate anything.

G) It might be possible to handle expiration of authorization via a general .onError, but would be up for any proposed more specific handling of this.

H) That is a great question, and I think the best bet now is to, as you said, leave it as an implementation-dependent feature set. I will add a note to that effect.

I) Yes, thank you for catching that.

J) I will add to all the parameter tables.

K) I will defer to the VISS spec. If it's aMUST there, then I agree that it should be a MUST here.

I'll begin working on addressing B, C, E, H, I, and J right away and we can continue discussing the others.


  • B - Errors from the VISS will be forwarded directly, client-side errors may be implementation-dependent
  • C - remove client from connect callback
  • E - value should be the payload from the VISS server
  • H - Note that the byCSSPath and other VISS tree traversal feature sets may be implementation dependent
  • I - openWindow and openAllWindows
  • J - Add types to parameter tables

…backs, adding clarification to a few other items.
@aShinjiroUrata
Copy link
Contributor

Hi, Powell-san,
Thanks for the response !
I'll review your response and updates one by one.
Excuse me that some of my previous comments seems based on my wrong understanding.

@aShinjiroUrata
Copy link
Contributor

aShinjiroUrata commented Mar 21, 2017

Hi Powell-san,
Thanks for the change.

  • B, C, E, H, I, J are OK to me by 847b86b change.
  • A is also OK to me.
    (= no need to set 'sub-protocol = wvss1.0' at creating W3CClient object. Sub-protocol setting is included inside VIAS library.)
    The rest D, F, G, K are not solved yet by Powell-san's last modification.

Since this pull request is submission of respec version of Vehicle Information API Spec,
I suppose it will be better to

  • merge pull request and finish this discussion.
  • Create separate github issues for D, F, G, K. (I'll copy the discussion and create issues.)

By following above strategy, this pull request is OK to merge for me.

@aShinjiroUrata
Copy link
Contributor

As remaining issues are already separated as github issues, I think this Pull Request is ready to merge.
Please comment if any of you have other issues.
If there is not comments, I'd like to merge this.

@rstreif
Copy link
Contributor

rstreif commented Mar 28, 2017

@aShinjiroUrata I am ok with merging @pkinney 's pull request.

@aShinjiroUrata
Copy link
Contributor

Rudi-san Thanks for the comment.
I noticed that I don't have 'write access' to this repository
and so I can't merge by myself.

Could someone merge this PR? Or may be grant me 'write access'?

@acrofts84
Copy link
Contributor

Hi Urata-san,
As discussed, this looks good to me. I believe Ted has the permissions to add another editor to the repo.

@acrofts84 acrofts84 merged commit 75f815a into gh-pages Mar 29, 2017
@tguild
Copy link
Member

tguild commented Mar 29, 2017

Urata-san, I have increased your permissions in this repository.

@aShinjiroUrata
Copy link
Contributor

Adam-san, Ted-san, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants