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 3 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
Contributor

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
Contributor

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.

@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.

@mplsmitch
Copy link
Collaborator Author

@LaurentG-AMD
I think for your example above you could do this using multiple policies:

{
  "data": {
		"policies": [{
			"curb_policy_id": "A",
			"published_date": 1552678857362,
			"priority": 1,
			"time_spans": [{
				"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
			}],
			"rules": [{
				"activity": "parking"
			}]
		}, {
			"curb_policy_id": "B",
			"published_date": 1552678857362,
			"priority": 2,
			"rules": [{
				"activity": "no parking"
			}]
		}]
	}
}

Policy A allows parking 9h00-11h00 on Fridays, April 1st to November 30th, but not during snow removal. Policy B prohibits parking at all times. Since Policy A has a lower priority it would take precedence over B during the Friday 9-11 timespan.
Does this seem like it would work?

Also, are you still interested in exceptions to user classes? Maybe we could add it to this PR.

@schnuerle
Copy link
Member

Everyone please take a look ASAP at this PR #136 for final recommended additions of 2 new exception fields and descriptions for user classes and time spans for issues #117 and #124.

@schnuerle schnuerle linked an issue Nov 19, 2024 that may be closed by this pull request
@jacobmalleau
Copy link
Collaborator

Both the except additions look good on my end, particular the user class one which should help us minimize the long list of user classes (something @RickNeubauer has brought up before.

@LaurentG-AMD
Copy link

@mplsmitch Sorry I did not see your post in August.

Your proposed combination is doing exactly the opposite of what my example needs (which is prohibiting all days except fridays), but I'm not debating that we could achieve it via 2 policies. Rather, my point is that we should aim for a single policy per sign so maintaining the database is easier when regulations evolve. I'm not sure my example was the best to showcase this since it was mixing a permanent exception with an exceptional one and those tend to appear on different signs.

Here's a better one: take my sign above and change the mention of "friday" for "friday and saturday". I'm pretty sure if i dig long enough I could find a real exemple of such a sign somewhere in the streets here. We still can't really write it efficiently with the proposed implementation: while in "days_of_week", we can enter every days where it applies in an array, "designated_period" is specifically asking for a string. Now changing the type from string to list is surely a breaking change, but in the mean time both could be accepted without breaking any existing implementation (tough I feel we should put up a mention that this is going to a strait array for the next major release).

This would now read (changes in bold):

Name Type Required/Optional Description
designated_period String or Array of strings Optional A string, or array of, representing an arbitrarily-named, externally-defined period of time. An array should be used if more than one period apply. Any values MAY be specified but the following known values SHOULD be used when possible:
  • snow emergency
  • holidays
  • school days
  • game days
Warning: the Type is expected to be modified to "Array of strings" in a future major release.

My above exemple (friday + snow removal) would now be entered as:

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

and 2 days exceptions would become

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

@schnuerle
Copy link
Member

schnuerle commented Dec 3, 2024

Thanks @LaurentG-AMD

@jacobmalleau @jiffyclub @mplsmitch what do you think about the proposed changes by @LaurentG-AMD in the comment?

Just to document here, I do think the designated_period field should be of type "Array of Strings", like we do with user_classes and other fields in the specs, in the next major release. If it can be an array, then it's always an array, and not just a string sometimes, even if it's just one value.

But now I think this kind of proposed change is not a patch, but would be in a minor release, so 1.1. This is more than just a clarification, but instead adding new functionality (that is not breaking in Laurent's suggestion). Plus we are adding a new user_classes_except field, which is fine to do in a minor release, but not a patch.

@schnuerle schnuerle modified the milestones: 1.0.1, 1.1 Dec 3, 2024
@jacobmalleau
Copy link
Collaborator

@schnuerle I generally agree with what @LaurentG-AMD proposed.

My only additional thought, and this is likely just a preference depending on the city, is that ensuring you can uniquely define every type of sign with a unique policy is not always beneficial when defining policies city wide.
Using the example above, a snow removal policy likely applies city wide, where as the Friday No Parking restriction is likely more local and changes depending on the area (no parking Saturday, other week days/times elsewhere). Now by making these all unique policies, if you ever needed to update the snow removal policy (e.g. adjust the date from April 1 to March 1) you would have to update multiple policies instead of just one policy. Not necessarily a bad thing, but potentially adds more complexity. However, with what is currently is proposed, there is nothing stopping someone from still making two separate policies in this example.

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 User Classes Representing Exception TimeSpans
6 participants