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: recommended file notice #1151

Conversation

KClough
Copy link
Collaborator

@KClough KClough commented May 16, 2022

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!

  • 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)

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

@CLAassistant
Copy link

CLAassistant commented May 16, 2022

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

isabelle-dr commented May 18, 2022

@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
Collaborator Author

KClough commented May 18, 2022

@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
@maximearmstrong maximearmstrong added this to In Review in The Tech Dashboard (archived) via automation May 27, 2022
@KClough KClough force-pushed the feat/recommended-file-notice branch 2 times, most recently from 1740012 to 03c38bd Compare June 24, 2022 02:16
Copy link
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!

@@ -0,0 +1,31 @@
/*
* Copyright 2020 Google LLC
Copy link
Contributor

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
Contributor

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.

The Tech Dashboard (archived) automation moved this from In Review to Approved Jun 28, 2022
@maximearmstrong
Copy link
Contributor

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

@KClough KClough force-pushed the feat/recommended-file-notice branch from 3eab428 to febfe9f Compare June 29, 2022 19:01
@KClough
Copy link
Collaborator 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
Contributor

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
Contributor

Thank you @KClough!

@maximearmstrong maximearmstrong merged commit c7787be into MobilityData:master Jun 29, 2022
The Tech Dashboard (archived) automation moved this from Approved to Done Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants