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

Clarify designated_period_except definition #136

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

mplsmitch
Copy link
Collaborator


name: Mitch Vars
title: Clarify designated_period_except definition


CDS Pull Request

Explain pull request

The current definition of designated_period_except is unclear in that it only covers values named in designated_period. This PR expands the definition to cover all fields in the Time Span. For example, If days_of_week is ['sun'], the Time Span does not apply on Sundays.
Additional info in #117

Is this a breaking change

  • No, not breaking

Impacted Spec

Which API(s) will this pull request impact?

  • Curbs

@mplsmitch mplsmitch requested a review from a team as a code owner March 11, 2024 20:39
@schnuerle schnuerle added this to the 1.0.1 milestone Mar 26, 2024
@schnuerle schnuerle added Curbs API minor update A change that is minor and should require little discussion, or is a maintenance/readme/typo update. labels Mar 26, 2024
@schnuerle schnuerle self-assigned this Mar 26, 2024
@schnuerle schnuerle linked an issue Mar 26, 2024 that may be closed by this pull request
@schnuerle
Copy link
Member

Can your teams take a look at this rewording @jacobmalleau @pierre-bouffort @jiffyclub and make sure that this change does not require anyone to redo their existing code, and that it's not breaking?

@pierre-bouffort
Copy link
Collaborator

Hello @schnuerle and thanks for pointing it out.
We checked that internally and this is not a breaking change for us so we are good to proceed (from our perspective at least).

@jiffyclub
Copy link

Looks good to me.

@jacobmalleau
Copy link
Collaborator

It also seems fine to me - this is a welcome addition to the spec.

curbs/README.md Outdated Show resolved Hide resolved
@jiffyclub
Copy link

jiffyclub commented Apr 9, 2024

This does now mean that the designated_period_except field is misnamed since we can use it to indicate exceptions for things other than designated periods, but fixing that up would be a breaking change that maybe we want to do in another release. Potential better names: exception, exception_time_span.

Co-authored-by: Matt Davis <jiffyclub@gmail.com>
@schnuerle
Copy link
Member

This does now mean that the designated_period_except field is misnamed since we can use it to indicate exceptions for things other than designated periods, but fixing that up would be a breaking change that maybe we want to do in another release. Potential better names: exception, exception_time_span.

You may be right here. If we do rename this field then it will have to wait for a 2.0 major release instead of a 1.1.0 patch. Is there some way to clarify this in the field description instead for now?

@LaurentG-AMD
Copy link

LaurentG-AMD commented Jun 3, 2024

I feel this changes the way designated_period is used and might make the initial usage hard to achieve in certain cases.

The signs I had in mind when proposing this change are these:
image
which litteraly reads as No Parking Except 9h00-11h00 on Friday from April 1rst to November 30th (I have no idea why parking is only permitted 2 hours during the week but those are real signs I will eventually need to fit in the data model). The current way of coding it would require to make at least 3 Time Spans: "No Parking Sat-Thu" + "No Parking Fri 0-9" + "No Parking Fri 11-24" which is functional when reading the data but make it harder to keep in line with changes when scaled city wide.

Now, there's nothing preventing us to also want to have an exception for snow removal (or whatever reason), where we might want to override it with something else. But in this case, the exception would already be used, so how do we interpret the interaction?

Taking the exemple sign above:

 {
  "months": [4,5,6,7,8,9,10,11],
  "days_of_week": ["fri"],
  "time_of_day_start": "9:00",
  "time_of_day_end": "11:00",
  "designated_period": "Snow removal",
  "designated_period_except":True
}

What happens if we have snow removal on Friday November 15th (of whatever year that happens), is it still a permitted parking or does it become a no_parking? And what about Saturday November 16th where snow operations are still ongoing, can I now park because the sign is voided or is it still a no parking?

I feel if we want to keep both functionalities, they need to use their own exception fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Curbs API minor update A change that is minor and should require little discussion, or is a maintenance/readme/typo update.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Representing Exception TimeSpans
6 participants