usdot-jpo-ode / wzdx

The Work Zone Data Exchange (WZDx) Specification aims to make harmonized work zone data provided by infrastructure owners and operators (IOOs) available for third party use, making travel on public roads safer and more efficient through ubiquitous access to data on work zone activity.
Creative Commons Zero v1.0 Universal
89 stars 62 forks source link

WZDx v4.0 #221

Closed j-d-b closed 2 years ago

j-d-b commented 2 years ago

WZDx v4.0

WZDx version 4.0 implements clean up and small additions in functionality to the WZDx feed and adds definitions for two new feeds, the SwzDeviceFeed and RoadRestrictionFeed. Until version 4.0, the WZDx specification defined only one feed, the WZDxFeed.

Features

Refactoring

j-d-b commented 2 years ago

I added release notes—note that the content of the Release Notes section in the project README is identical to the v4.0 section in RELEASES.md and the first PR comment above.

I was thinking we should slim down to the Release Notes section of the project README and leave the detailed notes in the RELEASES.md, what do you think?

mark-mockett commented 2 years ago

I was thinking we should slim down to the Release Notes section of the project README and leave the detailed notes in the RELEASES.md, what do you think?

I agree, especially considering how long this version's notes are.

j-d-b commented 2 years ago

I agree, especially considering how long this version's notes are.

I revised the Release Notes section of the project README to remove some specifics (such as the sub-lists).

chuehlien commented 2 years ago

Added a couple of comments. Please double check my changes, since I'm not 100% sure about some of the ones I've made relating to the field device that referenced RoadEvents. I've only reviewed the RoadRestrictionFeed and WZDxFeed contents and have not reviewed the json schema, example files, and only skimmed the example README - hope others are able to provide coverage for those. Good work, all!

j-d-b commented 2 years ago

Added a couple of comments. Please double check my changes, since I'm not 100% sure about some of the ones I've made relating to the field device that referenced RoadEvents.

@chuehlien your review was excellent and fixed a lot of issues. Thank you so much. I updated two of your edits regarding the use of the TrafficSensorLaneData (this object's tie to the road event is a bit confusing).

If you can make any time to review the schemas at any point, even if it's after the release, I'd really appreciate it as I bet you would find some issues given what you found in the markdown files.

Eventually, I think it would be ideal if the JSON schema is all that we maintain and we can auto-generate more human-friendly docs from it assuming there are acceptable tools for that (e.g. json-schema-for-humans).

j-d-b commented 2 years ago

should the WZDx feed also be changed to feed_info?

I would like that, but I brought that up at a spec update co-chairs meeting (near the end of the meeting) and at that time the consensus was (which I understood) that it was another breaking change for minimal reason.

However seeing that we don't want to do another major release anytime soon and the change would great greater consistency and less stuff that is only that way for historical reasons, I think we should do it. It's a trivial from a development perspective.

@mark-mockett, @sknick-iastate I think if you both are for it (renaming the WZDxFeed road_event_feed_info property to feed_info for consistency with the RoadRestrictionFeed and SwzDeviceFeed), let's do it.

sknick-iastate commented 2 years ago

should the WZDx feed also be changed to feed_info?

I would like that, but I brought that up at a spec update co-chairs meeting (near the end of the meeting) and at that time the consensus was (which I understood) that it was another breaking change for minimal reason.

However seeing that we don't want to do another major release anytime soon and the change would great greater consistency and less stuff that is only that way for historical reasons, I think we should do it. It's a trivial from a development perspective.

@mark-mockett, @sknick-iastate I think if you both are for it (renaming the WZDxFeed road_event_feed_info property to feed_info for consistency with the RoadRestrictionFeed and SwzDeviceFeed), let's do it.

Just throwing it out there but what are your thoughts on changing the restriction to road_event_feed_info? Then also changing the SwzDevice to swz_device_feed_info? I know it breaks the consistency but really these would not be in the same feed since it is different producers/consumers.

j-d-b commented 2 years ago

Just throwing it out there but what are your thoughts on changing the restriction to road_event_feed_info? Then also changing the SwzDevice to swz_device_feed_info?

Since the properties use the same object (the FeedInfo object), I think it is more clear to have the property name be the same for all feeds.

In addition, the WZDxFeed could be expanded to include things that are not road events and then the road_event_feed_info would be incorrect.

mark-mockett commented 2 years ago

@j-d-b @sknick-iastate I agree that it'd be cleaner to have each feed info property called the same thing. But I'm also reluctant to make several last-minute revisions that don't improve functionality of the spec (applies to this requiring LineString geometry for detours)

mark-mockett commented 2 years ago

@j-d-b I need some help understanding what I (accidentally) did with the "Merge branch v4.0 release" commit and its reversion - I switched over to the desktop version of GitHub, and it looks like the first commit just redid all the changes that have been made since the last merge (even though they were already committed). I reverted it but now all those changes are gone. Do I revert the reversion??

sknick-iastate commented 2 years ago

@j-d-b should the road_event_id be removed from the MarkedLocation Object? It seems redundant since the FieldDeviceCoreDetails already includes the road event id's. I can't think of a scenario where this information would be different but wanted to verify in case I was overlooking something.

j-d-b commented 2 years ago

I agree that it'd be cleaner to have each feed info property called the same thing. But I'm also reluctant to make several last-minute revisions that don't improve functionality of the spec (applies to this requiring LineString geometry for detours)

I think the only revisions we should make right now are those that don't change the functionality of the spec! As for the specific proposal of renaming road_event_feed_info to feed_info, I'll make an issue for it to address next cycle.

should the road_event_id be removed from the MarkedLocation Object?

Good thought and I'd like to continue the discussion but I think this is too major of a change to make now. Can you make an issue for this?

sknick-iastate commented 2 years ago

@j-d-b sounds good on the MarkedLocation issue. I have a couple others written out as well but was waiting for v4.0 release to be merged so I can link to the relevant information directly.

j-d-b commented 2 years ago

I need some help understanding what I (accidentally) did with the "Merge branch v4.0 release" commit and its reversion...

@mark-mockett I see you reverted the revert commit. That was the right call. It looks like the merge commit was just catching up/combining your local copy to what was new on remote since last time you were working locally.