-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix off-by-one-error in sat range template #4075
Conversation
d207a55
to
5eccc18
Compare
Wouldn't it be more convenient for consumers of the API if the JSON output also reflected the change to inclusive intervals? |
@arik-so I think that's a separate issue. The HTML is straightforwardly wrong, since we link directly to the sat which is the end of the interval, and this fixes that. I think the JSON API is kind of a separate issue, since I think there there's more of an argument that it's a programming interface, and so you want half-open ranges. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is pretty confusing. We have had the sat ranges being shown as half open intervals since launch and now the meaning of the sat range has been changed to be a closed interval with no visual indication that the meaning has changed. This seems dangerous. At this point, people are used to the ranges being half open. I would suggest not using the same syntax for half open ranges and closed ranges. |
@gmart7t2 I agree this sucks. However, I think we were in an unfortunately situation where people who are used to half-open ranges knew what the template meant, and people who weren't were confused, so I'm not sure this change is a negative. I'm also not sure how we could change the notation without making it worse. |
Sat ranges are no longer intervals, and instead link to the first and last sat in the range. However, the last sat is off by one. Fix that.