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

Refactor event affordance and event type naming #251

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

farshidtz
Copy link
Member

@farshidtz farshidtz commented Dec 19, 2021

This PR changes the event type's naming convention from Convention B (e.g. create) to Conversion C (e.g. thing_created); see #197.

Adding the data type is to allow extensions to the notification API to deliver event data other than just TDs.
Changing from action verb to past verb is to make the event type more meaningful when associated with delivered events, since the type value is also to identify the type of event data on the stream.

It closes #197


In addition, it changes the event affordance names from nouns to past verbs for consistency with event type values.

It closes #258


Preview | Diff

Copy link
Member

@benfrancis benfrancis left a comment

Choose a reason for hiding this comment

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

I really like changing the event names from "create" to "thing created" as I think that makes a lot more sense from a linguistic point of view, however I think the affordance names should be updated as well as just the URLs. The fact that the affordance name and URL use different wording is confusing.

Also note that using an underscore as a separator would mean there would be three different conventions for multi-word strings in a Directory Thing Description:

  1. Lower camel case for event affordance names (e.g. thingCreation)
  2. All lower case for operation names (e.g. subscribeevent)
  3. Underscore separated for event URLs (e.g. thing_created)

I would suggest that the wording in the affordance name should at least match the wording in the URL, and ideally the case would match too, e.g. they could both be thingCreated.

My real preference would be to omit the URLs from the Thing Model (#253) altogether and let them be implementation-specific, but I realise we've already discussed that at some length and that is complicated by URL variables.

@farshidtz
Copy link
Member Author

I really like changing the event names from "create" to "thing created" as I think that makes a lot more sense from a linguistic point of view, however I think the affordance names should be updated as well as just the URLs. The fact that the affordance name and URL use different wording is confusing.

The affordance names (thingCreation, thingUpdate, and thingDeletion) are nouns, following the TD convention for event affordances. I have no objection to changing those (to thingCreated, thingUpdated, and thingDeleted) if there is consensus, but that is unrelated to this PR.

Also note that using an underscore as a separator would mean there would be three different conventions for multi-word strings in a Directory Thing Description:

  1. Lower camel case for event affordance names (e.g. thingCreation)
  2. All lower case for operation names (e.g. subscribeevent)
  3. Underscore separated for event URLs (e.g. thing_created)

There is already more than three different conventions in the TD, some of them out of our control.

But in URLs, we have consistently followed snake_case. E.g. sort_by and sort_order. I don't mind changing all to kebab-case.

Other snake_case instances are in the security definitions:

"oauth2_code": {

I would suggest that the wording in the affordance name should at least match the wording in the URL, and ideally the case would match too, e.g. they could both be thingCreated.

I think adding case-sensitive parts to URLs will add complication for both server and clients.

@farshidtz
Copy link
Member Author

From Discovery call. There is consensus on

  • adding the object type to the event name
  • having a separator e.g. snake_case or camleCase

We'll wait one more week to collect more feedback.

@benfrancis
Copy link
Member

@farshidtz wrote:

The affordance names (thingCreation, thingUpdate, and thingDeletion) are nouns, following the TD convention for event affordances.

I have never heard of this convention. I always use nouns for property names, verbs for action names and past verb for event names. I notice that the Thing Description specification currently uses "overheated" in one place and "overheating" in another.

I have no objection to changing those (to thingCreated, thingUpdated, and thingDeleted) if there is consensus, but that is unrelated to this PR.

OK, I've filed #258 for that.

I think adding case-sensitive parts to URLs will add complication for both server and clients.

I don't think they necessarily have to be case sensitive, but I agree it could be a source of errors. If the affordance names at least match the wording in the URLs (#258) then I wouldn't strongly object to the URLs using snake_case while the affordance names use lowerCamelCase.

@AndreaCimminoArriaga
Copy link
Contributor

I think using the under score (snake_case) as separator could be a better solution since URLs are case insensitive and, therefore, using camel case may not be as suitable as the '_' separator.

@farshidtz
Copy link
Member Author

I'm going to change this to draft to first:

@farshidtz farshidtz marked this pull request as draft January 17, 2022 15:31
@farshidtz farshidtz force-pushed the notification-event-type branch from 7d8bdd2 to 9adbf4c Compare January 24, 2022 13:55
@farshidtz
Copy link
Member Author

Rebased to resolve conflicts. The changes are now applied to the TM instead of the TD.

This changes the event affordance names from nouns to past verbs.
This is to make the affordance names consistent with event types:

| Event Affordance | Event Type    |
|------------------|---------------|
| thingCreated     | thing_created |
| thingUpdated     | thing_updated |
| thingDeleted     | thing_deleted |

Closes w3c#258
@farshidtz farshidtz changed the title Change event type's naming convention to thing_<past-verb> Refactor event affordance and event type naming Jan 24, 2022
@farshidtz farshidtz requested a review from benfrancis January 24, 2022 14:11
@farshidtz farshidtz marked this pull request as ready for review January 24, 2022 14:13
@mmccool mmccool merged commit 47ce754 into w3c:main Jan 24, 2022
@farshidtz farshidtz deleted the notification-event-type branch March 7, 2022 16:13
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.

Change event affordance names Improving event type values in the Notification API
4 participants