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

feat: add StopTimeTripWithoutTimes validator #1474

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

Conversation

KClough
Copy link
Collaborator

@KClough KClough commented Jun 2, 2023

Summary:

Summarize the changes in the pull request including how it relates to any issues (include the #number, or link them).

Partially resolves #1089

Expected behavior:

Adds a new validator that reports an error if a trip includes stop time that are missing either arrival time or departure time.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

✅ Rule acceptance tests passed.
New Errors: 0 out of 1428 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1428 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1428 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1428 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1428 sources (~0 %) are corrupted.
Commit: e527821
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

Copy link
Collaborator

@briandonahue briandonahue left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +326 to +327
* We can't generate the ERROR notice block_trips_with_overlapping_stop_times for the same trip
* because we are missing time information
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that stop times are missing and there is no error?

@isabelle-dr can you review this wording?

@isabelle-dr
Copy link
Contributor

If I understand properly, this notice is different from the other ones in the sense that it informs that a notice could not have been checked.

I saw that this was briefly discussed in the issue related, but this is a broader question of what to do when portions of this validator don't run (and a notice could not have been checked). This is the case for plenty of these validator notices and I think they should be treated differently since it's not technically a spec violation.

@isabelle-dr
Copy link
Contributor

isabelle-dr commented Jun 5, 2023

We should probably have discussed this in a bit more detail before opening the PR, sorry @KClough. 😕

I believe there are preferred ways to solve this problem, with a section in the validator report that informs of the list of notices that could not have been checked.

@isabelle-dr isabelle-dr added the status: Needs discussion We need a discussion on requirements before calling this issue ready label Jun 5, 2023
@KClough
Copy link
Collaborator Author

KClough commented Jun 6, 2023

@isabelle-dr I assumed we'd end up doing both:

  • alerting a user that their stop times data is missing information
  • building a section in the validator report for notices that could not be checked

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

✅ Rule acceptance tests passed.
New Errors: 0 out of 1430 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1430 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1430 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1430 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1430 sources (~0 %) are corrupted.
Commit: ae14cc8
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

@emmambd emmambd requested a review from isabelle-dr June 9, 2023 14:38
@isabelle-dr
Copy link
Contributor

@KClough

  • alerting a user that their stop times data is missing information
  • building a section in the validator report for notices that could not be checked

We are aligned there on the second point.
For the first one, I suggest we also put this piece of information in the new section the section that informs the user that some notices could not have been checked, because it informs the user of why the notice couldn't have been checked. ideally.

I would like to keep the list of notices in the "Specification Compliance report" section as true spec violations. If not, we risk making this section more noisy, which is an incentive to ignore it.

Sorry for the misalignment there, I hope we can recover some of the work in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: Needs discussion We need a discussion on requirements before calling this issue ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlockTripsWithOverlappingStopTimesNotice error not being caught
4 participants