Replace feed_expiration_date with rules for 7 and 30 days #1245

KClough · 2022-08-24T20:06:54Z

Summary:

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

Resolves #930

Expected behavior:

Explain and/or show screenshots for how you expect the pull request to work in your testing (in case other devices exhibit different behavior).

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)

maximearmstrong

LGTM! It looks like this branch is out-of-date with master and I don't have the Update branch button option. Could you update it so we can merge the PR? Thanks @KClough

KClough · 2022-08-25T00:18:51Z

@isabelle-dr just a heads up that I did not split the rules as you proposed in #930 as that seems like it'd only offer additional utility if #1117 was also implemented.

maximearmstrong

I've just realized we should update RULES.md to reflect the changes before merging.

@isabelle-dr just a heads up that I did not split the rules as you proposed in #930 as that seems like it'd only offer additional utility if #1117 was also implemented.

That being said and considering this comment ☝️, I'm also wondering if this notice should be split in two as Isabelle has suggested. From a technical point of view, this PR makes a lot of sense. I think we should just make sure this is the most valuable approach for users.

Specifically, I agree with @gcamp 's comment here, but having one notice for both threshold would make it the only notice to appear as both WARNING and INFO in RULES.md. This is not necessarily a problem, but it could be confusing for users if the difference isn't very well described.

@isabelle-dr, what are your thoughts on this?

I see two options here:

We keep the code as is and we add the same notice but with a different description under INFO in RULES.md. We would also need to update the current notice description under WARNING.
We change the code to add a second notice and update RULES.md to reflect the changes.

isabelle-dr · 2022-08-30T20:47:10Z

Hello, @KClough, thanks for tackling this issue!

I am generally against having two severities for the same notice, for the following two reasons:

some users filter through the validator output based on the notice name. If a user wants to filter datasets based on the feed expiration notice, having what is proposed in this PR is better than what we currently have (the user would filter based on notice presence + severity), but the best would be to filter based on the notice presence.
the severities could become user-defined one day (referencing the custom validation feature). One consumer could update the severity for a rule if it's a requirement for their trip planning app, for example. We aren't yet sure what refactor would the validator need to support this, but it could mean that the severities are defined in another file.

I would suggest the code here outputs two rules, maybe feed_expiration_date_7_days and feed_expiration_date_30_days?
As weird as it might sound, I suggest both of these to be set to WARNING because it is based on the Best Practices.
That being said, having both of them triggered for a dataset that has an expiration date in less than 7 days doesn't make sense.

IMHO the behavior of this check should be:

if feed_info.feed_end_date <= today + 7, trigger feed_expiration_date_7_days with severity WARNING
if [feed_info.feed_end_date > today + 7] AND [feed_info.feed_end_date <= today + 30] -> feed_expiration_date_10_days with severity WARNING

isabelle-dr · 2022-08-31T13:47:31Z

My message weirdly got posted twice after I edited it, I just deleted one to keep the latest version 😊

isabelle-dr

@KClough, thanks for updating this PR!
I have made suggestion changes in this PR, mainly:

Modify the names FeedExpirationDateNotice30Days to FeedExpirationDate30DaysNotice so that they render properly in the validation report (currently it ends-up being feed_expiration_date_notice30_days). I think we will still have a slight problem where they will look like feed_expiration_date30_days.
Updates to the documentation so that every rule has all the sections we need.

Additionally, can you please:
3. Rename this PR to "replace feed_expiration_date with rules for 7 and 30 days".
4. Update the table at the top of RULES.md with the new notices.

You can add Jarvus Innovations LL in the copyright section at the top of the files 😊

RULES.md

isabelle-dr · 2022-09-15T16:06:43Z

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidator.java

+      this.csvRowNumber = csvRowNumber;
+      this.currentDate = currentDate;
+      this.feedEndDate = feedEndDate;
+      this.suggestedExpirationDate = suggestedExpirationDate;


Do we need to specify a suggested expiration date here? The real suggestion is for as long as the operator is confident that the schedule will continue to be operated, as per the Best Practices.
I see a risk of sending the wrong message, and not much added value.
@bdferris, thoughts?

Maybe we don't need it, but if you look at the original FeedExpirationDateNotice, the field was already included. If we decide to remove it, maybe it we should open another PR to make the refactor.

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidator.java

.../src/test/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidatorTest.java

isabelle-dr · 2022-09-15T16:47:39Z

.../src/test/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidatorTest.java

@@ -69,7 +70,7 @@ public void feedExpiringInSevenDaysFromNowShouldGenerateNotice() {
            validateFeedInfo(
                createFeedInfo(GtfsDate.fromLocalDate(TEST_NOW.toLocalDate().plusDays(7)))))
        .containsExactly(
-            new FeedExpirationDateNotice(
+            new FeedExpirationDateNotice30Days(


Suggested change

new FeedExpirationDateNotice30Days(

new FeedExpirationDate30DaysNotice(

.../src/test/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidatorTest.java

isabelle-dr · 2022-09-15T17:09:50Z

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/FeedExpirationDateValidator.java

@@ -55,7 +56,7 @@ public void validate(GtfsFeedInfo entity, NoticeContainer noticeContainer) {
      GtfsDate currentDatePlusThirtyDays = GtfsDate.fromLocalDate(now.plusDays(30));
      if (entity.feedEndDate().isBefore(currentDatePlusSevenDays)) {


Why is this formula different than the one we have for 30 days?
I have tested a dataset that had feed_end_date == [today + 7], which should trigger the 7 days notice, but it the 30 days notice was emitted. I think with this formula, what we trigger is a 6 days threshold.

cc @bdferris

@isabelle-dr I believe you're correct, there is a bug with the 7 day calculation. I updated my PR to fix this issue and the related test.

yup, it works now :)

isabelle-dr · 2022-09-21T20:59:44Z

RULES.md

+* [General Publishing & General Practices](https://gtfs.org/best-practices/#dataset-publishing--general-practices)
+
+<details>
+#### Notice fields description


this table doesn't render, I think we need a line break after "Notice fields description".

isabelle-dr

Besides my last suggestion (replace 6 days by 7 days) and the rendering of the "Notice fields description" section for the 7 days notice in RULES.md, this PR looks good to me 👍.

@bdferris, do you have the capacity to do the code review for this PR, and do you know what would be the fix so that the notice code doesn't appear as feed_expiration_date7_days, but appears as feed_expiration_date_7_days instead?

maximearmstrong

Aside from the changes requested by @isabelle-dr in RULES.md, LGTM! Thanks @KClough

KClough · 2022-09-23T20:06:41Z

@maximearmstrong I have resolved this feedback, please re-review when you get a chance. Thanks!

Co-authored-by: isabelle-dr <[email protected]>

…edExpirationDateValidator.java Co-authored-by: isabelle-dr <[email protected]>

…edExpirationDateValidatorTest.java Co-authored-by: isabelle-dr <[email protected]>

isabelle-dr · 2022-09-30T22:56:46Z

LGTM! Thanks Kevin for updating this PR

KClough requested review from bdferris-v2 and maximearmstrong August 24, 2022 20:07

maximearmstrong approved these changes Aug 24, 2022

View reviewed changes

KClough force-pushed the issue/930/feed-expiration-date-notice-info-30-days branch from 40c94f4 to 62a6fbe Compare August 25, 2022 00:15

maximearmstrong suggested changes Aug 25, 2022

View reviewed changes

KClough force-pushed the issue/930/feed-expiration-date-notice-info-30-days branch from 62a6fbe to 0060ae0 Compare September 4, 2022 21:09

isabelle-dr requested changes Sep 15, 2022

View reviewed changes

KClough changed the title ~~feat: Apply info level severity to feed expiration date of less than 30 days~~ Replace feed_expiration_date with rules for 7 and 30 days Sep 16, 2022

KClough force-pushed the issue/930/feed-expiration-date-notice-info-30-days branch from a75d754 to effaead Compare September 16, 2022 20:43

isabelle-dr reviewed Sep 21, 2022

View reviewed changes

isabelle-dr approved these changes Sep 21, 2022

View reviewed changes

maximearmstrong reviewed Sep 22, 2022

View reviewed changes

maximearmstrong approved these changes Sep 22, 2022

View reviewed changes

KClough force-pushed the issue/930/feed-expiration-date-notice-info-30-days branch from c5f8dbd to f9badba Compare September 23, 2022 20:06

KClough force-pushed the issue/930/feed-expiration-date-notice-info-30-days branch from f9badba to ca7a730 Compare September 29, 2022 15:33

KClough and others added 11 commits September 29, 2022 12:25

apply info severity to feed exp date <= 30 days

1e1c7eb

Update RULES.md

6e446a5

Co-authored-by: isabelle-dr <[email protected]>

Update RULES.md

9e66488

Co-authored-by: isabelle-dr <[email protected]>

Update main/src/main/java/org/mobilitydata/gtfsvalidator/validator/Fe…

96ed0f0

…edExpirationDateValidator.java Co-authored-by: isabelle-dr <[email protected]>

Update main/src/test/java/org/mobilitydata/gtfsvalidator/validator/Fe…

12e9ae6

…edExpirationDateValidatorTest.java Co-authored-by: isabelle-dr <[email protected]>

Update main/src/test/java/org/mobilitydata/gtfsvalidator/validator/Fe…

3243868

…edExpirationDateValidatorTest.java Co-authored-by: isabelle-dr <[email protected]>

Update main/src/test/java/org/mobilitydata/gtfsvalidator/validator/Fe…

5e6a95a

…edExpirationDateValidatorTest.java Co-authored-by: isabelle-dr <[email protected]>

complete rule renaming

601dfbf

fix issue with 7 day expiration calculation

06ca44e

fix issue with table rendering

b12047b

apply markdown table format

e31f2d6

KClough force-pushed the issue/930/feed-expiration-date-notice-info-30-days branch from 1b2daf2 to e31f2d6 Compare September 29, 2022 16:25

KClough added 2 commits September 30, 2022 12:09

fix typo

df9d47b

split rule

74ce84e

isabelle-dr merged commit 099ae89 into MobilityData:master Sep 30, 2022

KClough deleted the issue/930/feed-expiration-date-notice-info-30-days branch October 3, 2022 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace feed_expiration_date with rules for 7 and 30 days #1245

Replace feed_expiration_date with rules for 7 and 30 days #1245

KClough commented Aug 24, 2022

maximearmstrong left a comment

KClough commented Aug 25, 2022

maximearmstrong left a comment

isabelle-dr commented Aug 30, 2022

isabelle-dr commented Aug 31, 2022

isabelle-dr left a comment

isabelle-dr Sep 15, 2022

maximearmstrong Sep 22, 2022 •

edited

Loading

isabelle-dr Sep 15, 2022

isabelle-dr Sep 15, 2022

KClough Sep 16, 2022

isabelle-dr Sep 21, 2022

isabelle-dr Sep 21, 2022

isabelle-dr left a comment

maximearmstrong left a comment

KClough commented Sep 23, 2022

isabelle-dr commented Sep 30, 2022

	new FeedExpirationDateNotice30Days(
	new FeedExpirationDate30DaysNotice(

		@@ -55,7 +56,7 @@ public void validate(GtfsFeedInfo entity, NoticeContainer noticeContainer) {
		GtfsDate currentDatePlusThirtyDays = GtfsDate.fromLocalDate(now.plusDays(30));
		if (entity.feedEndDate().isBefore(currentDatePlusSevenDays)) {

Replace feed_expiration_date with rules for 7 and 30 days #1245

Replace feed_expiration_date with rules for 7 and 30 days #1245

Conversation

KClough commented Aug 24, 2022

maximearmstrong left a comment

Choose a reason for hiding this comment

KClough commented Aug 25, 2022

maximearmstrong left a comment

Choose a reason for hiding this comment

isabelle-dr commented Aug 30, 2022

isabelle-dr commented Aug 31, 2022

isabelle-dr left a comment

Choose a reason for hiding this comment

isabelle-dr Sep 15, 2022

Choose a reason for hiding this comment

maximearmstrong Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

isabelle-dr Sep 15, 2022

Choose a reason for hiding this comment

isabelle-dr Sep 15, 2022

Choose a reason for hiding this comment

KClough Sep 16, 2022

Choose a reason for hiding this comment

isabelle-dr Sep 21, 2022

Choose a reason for hiding this comment

isabelle-dr Sep 21, 2022

Choose a reason for hiding this comment

isabelle-dr left a comment

Choose a reason for hiding this comment

maximearmstrong left a comment

Choose a reason for hiding this comment

KClough commented Sep 23, 2022

isabelle-dr commented Sep 30, 2022

maximearmstrong Sep 22, 2022 •

edited

Loading