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 Restrictions #198

Merged

Conversation

DeraldDudley
Copy link
Collaborator

@DeraldDudley DeraldDudley commented Aug 24, 2021

This PR resolves issue #155.

Changes reflect...

  • Making the Restriction object used by the RoadEvent Object
  • Changing name of LaneRestriction to Restriction
  • Add RoadEvent-RestrictionNotice Feature
  • Changed RoadRestriction to RetrictionType
  • Generalized wording of Restriction Types to not specifically reference the WorkZone Events
  • Changed LaneRestrictionUnit to UnitOfMeasurement
  • Uploaded Object Model and image to reflect changes (Can copy objects from my file to master file.)
  • Uploaded RoadEvent-RestrictionNotice Object Model (Can copy objects from my file to master file.)
  • Added and example

Copy link
Collaborator

@j-d-b j-d-b left a comment

Choose a reason for hiding this comment

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

I reviewed, made changes to the @DeraldDudley's v4.0-release-restrictions branch, re-reviewed, and now approve the changes.

I think we need another reviewer, as I originated several of the changes here and have a tendency to miss typos.

@j-d-b
Copy link
Collaborator

j-d-b commented Aug 25, 2021

The specification changes made by this PR are as follows:

  • Rename LaneRestrictionUnit enumerated type to UnitOfMeasurement which allows more general use
  • Rename the RoadRestriction enumerated type to RestrictionType which is more semantic
  • Rename the LaneRestriction object to Restriction which allows more general use
  • Remove the lane_restriction_ prefix from the properties of the Restriction object
  • Change units (on Restriction object) to be singular unit
  • Change the restrictions array on the RoadEvent object to be a list of the Restriction object rather than a list of the RestrictionType (the key change from this PR: road events can now have restrictions with a value and unit)

And in addition:

  • Update the example feeds to represent the specification changes above

Note that the object diagram was not updated which is the team's approach as we can't merge the diagram files so those changes need to happen after the PR is merged to the release branch. @DeraldDudley added new diagram files specific to these changes, but I removed those because we don't want those files merged in. @DeraldDudley if you have time, you can put the generated object diagram image in a comment in this thread, since you've already taken the time to make them. However, they will have to change based on the difference in the proposed changes from what you had originally.

@j-d-b j-d-b added the v4.0 label Aug 25, 2021
@j-d-b j-d-b changed the title V4.0 release - Restriction Pull Requests Refactor Restrictions Aug 26, 2021
@DeraldDudley
Copy link
Collaborator Author

Thanks for the review @j-d-b

Copy link
Contributor

@chuehlien chuehlien left a comment

Choose a reason for hiding this comment

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

Nice work! Minor updates needed for /spec-content/objects/Restriction.md and references to the old names need to be updated in /create-feed/README.md (lines 31 and 87).

spec-content/objects/Restriction.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chuehlien chuehlien left a comment

Choose a reason for hiding this comment

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

Looks great - great job @j-d-b @DeraldDudley !

Copy link
Collaborator

@sknick-iastate sknick-iastate left a comment

Choose a reason for hiding this comment

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

Great work @DeraldDudley and @j-d-b.

My only comment is that we should update the restrictions description in the Lane object table to match either the schema that was modified to "A list of zero or more restrictions specific to the lane" or similar to the road event level restrictions description of "A list of zero or more road restrictions that apply to the roadway segment described by this road event."

@j-d-b j-d-b added the Refactor This item related to refactoring the specification. label Oct 15, 2021
Updated RoadEvent-level restriction on 121388-WB example in comprehensive linestring file
@DeraldDudley
Copy link
Collaborator Author

Thanks for all the help. Sorry to be absent the past few weeks. My workload has been challenging to manage.

with Scenario 3 and multipoint examples
Copy link
Collaborator

@mark-mockett mark-mockett left a comment

Choose a reason for hiding this comment

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

I made a couple small updates to the examples for consistency, otherwise everything looks good!

@j-d-b
Copy link
Collaborator

j-d-b commented Oct 22, 2021

My only comment is that we should update the restrictions description in the Lane object table to match either the schema that was modified to "A list of zero or more restrictions specific to the lane" or similar to the road event level restrictions description of "A list of zero or more road restrictions that apply to the roadway segment described by this road event."

@sknick-iastate I made the change.

@j-d-b j-d-b added the New Functionality This item relates to adding new functionality to the specification label Nov 2, 2021
@mark-mockett mark-mockett merged commit 9a73683 into usdot-jpo-ode:v4.0-release Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Functionality This item relates to adding new functionality to the specification Refactor This item related to refactoring the specification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow restriction value and units for road event level restrictions
5 participants