Skip to content

feat: recommended file notice#1151

Merged
maximearmstrong merged 3 commits intoMobilityData:masterfrom
JarvusInnovations:feat/recommended-file-notice
Jun 29, 2022
Merged

feat: recommended file notice#1151
maximearmstrong merged 3 commits intoMobilityData:masterfrom
JarvusInnovations:feat/recommended-file-notice

Conversation

@KClough
Copy link
Copy Markdown
Contributor

@KClough KClough commented May 16, 2022 •
edited
Loading

Summary:

Related #877

Adds a new annotation for recommended files and applies this annotation to feed_info.txt and shapes.txt

Expected behavior:

A new validator warning is generated when either feed_info.txt is missing

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

@isabelle-dr per your request, this pull request contains just the notice.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 16, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@KClough KClough changed the title feat: recommended file annotation feat: recommended file notie May 16, 2022
@KClough KClough changed the title feat: recommended file notie feat: recommended file notice May 16, 2022
@isabelle-dr isabelle-dr mentioned this pull request May 18, 2022
Merged
3 tasks
@KClough KClough force-pushed the feat/recommended-file-notice branch from 9a249fc to 90818c5 Compare May 18, 2022 15:30
@isabelle-dr
Copy link
Copy Markdown
Contributor

isabelle-dr commented May 18, 2022 •
edited
Loading

@KClough could you remove the mention of shapes.txt in the description of this PR and update the image, since we decided to only apply this notice to feed_info.txt?
This is so that when we go back to this PR in the future, it is clear what it is doing. :)

@KClough
Copy link
Copy Markdown
Contributor Author

KClough commented May 18, 2022 •
edited
Loading

@KClough could you remove the mention of shapes.txt in the description of this PR and update the image, since we decided to only apply this notice to feed_info.txt? This is so that when we go back to this PR in the future, it is clear what it is doing. :)

Updated!
Thanks for catching that!
I got confused from shapes.txt mentioned in the original issue.

@KClough KClough force-pushed the feat/recommended-file-notice branch 3 times, most recently from 6e16db5 to 5179c0a Compare May 27, 2022 02:48
@KClough KClough force-pushed the feat/recommended-file-notice branch 2 times, most recently from 1740012 to 03c38bd Compare June 24, 2022 02:16
maximearmstrong
maximearmstrong approved these changes Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

One small comment aside, we are ready to merge this PR. Thank you @KClough!

core/src/main/java/org/mobilitydata/gtfsvalidator/notice/MissingRecommendedFileNotice.java Outdated
@@ -0,0 +1,31 @@
/*
* Copyright 2020 Google LLC
Copy link
Copy Markdown
Contributor

@maximearmstrong maximearmstrong Jun 28, 2022

Choose a reason for hiding this comment

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

You can add your organization to the copyright if you'd like and also update the year.

Copy link
Copy Markdown
Contributor

@maximearmstrong maximearmstrong Jun 28, 2022

Choose a reason for hiding this comment

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

If you change it, you can add [acceptance test skip] at the end of your commit message so the acceptance tests don't run once again.

@maximearmstrong
Copy link
Copy Markdown
Contributor

maximearmstrong commented Jun 28, 2022

Actually, if possible, the modifications applied to RULES.md here would be in this PR.

@maximearmstrong maximearmstrong mentioned this pull request Jun 28, 2022
Merged
3 tasks
@KClough KClough force-pushed the feat/recommended-file-notice branch from 3eab428 to febfe9f Compare June 29, 2022 19:01
@KClough
Copy link
Copy Markdown
Contributor Author

KClough commented Jun 29, 2022

Actually, if possible, the modifications applied to RULES.md here would be in this PR.

I didn't add the update to the Rules file in this PR because the Rule isn't actually applied to any datasets yet. Please confirm if you still think it makes sense to be in this PR.

@maximearmstrong
Copy link
Copy Markdown
Contributor

maximearmstrong commented Jun 29, 2022

I didn't add the update to the Rules file in this PR because the Rule isn't actually applied to any datasets yet. Please confirm if you still think it makes sense to be in this PR.

I think it's ok because it is partially mostly related to feed_infos.txt. In the future, let's say that it would better to add it in the PR where the notice itself is created.

As per the copyright, I believe it should be * Copyright 2020 Google LLC, Jarvus Innovations LLC as the code was partially took from the required annotation file. Sorry about the confusion!

@KClough KClough force-pushed the feat/recommended-file-notice branch from febfe9f to 8f36a17 Compare June 29, 2022 19:33
@maximearmstrong
Copy link
Copy Markdown
Contributor

maximearmstrong commented Jun 29, 2022

Thank you @KClough!

@maximearmstrong maximearmstrong merged commit c7787be into MobilityData:master Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@isabelle-dr isabelle-dr Awaiting requested review from isabelle-dr

1 more reviewer

@maximearmstrong maximearmstrong maximearmstrong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KClough @CLAassistant @isabelle-dr @maximearmstrong