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

Docs: Fix RULES.md notice name for 2 notices #1323

Merged
merged 3 commits into from
Feb 3, 2023
Merged

Conversation

isabelle-dr
Copy link
Contributor

@isabelle-dr isabelle-dr commented Feb 2, 2023

Summary:

Closes #1322

Expected behavior:

~~When either feed_expiration_date7_days or feed_expiration_date30_days is triggered, the link in the HTML report isn't broken. ~~
EDIT: the notice name in the validation report is the same than the notice name in RULES.md for these two notices.

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)

@emmambd
Copy link
Contributor

emmambd commented Feb 2, 2023

@isabelle-dr What's the best way to test this? It doesn't look like the rule link is broken within RULES.md in master.

@isabelle-dr isabelle-dr changed the title [Docs] Fix RULES.md Docs: Fix RULES.md Feb 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@isabelle-dr isabelle-dr changed the title Docs: Fix RULES.md Docs: Fix RULES.md notice name for 2 notices Feb 2, 2023
@isabelle-dr
Copy link
Contributor Author

isabelle-dr commented Feb 2, 2023

@emmambd you're right, the link isn't broken actually.

The user is manually scraping RULES.md to get the description of the notices (and I think they then match with what is found in the JSON report).

Here is a sample of the JSON report that this dataset creates, for example.

{"notices":[{
   "code":"feed_expiration_date7_days",
   "severity":"WARNING",
   "totalNotices":1

Then when parsing RULES.md with the notice code, the user calls https://github.com/MobilityData/gtfs-validator/blob/master/RULES.md#feed_expiration_date7_days which doesn't work because the name isn't accurate.

@lauriemerrell is this accurate, and is this PR solving the problem? Here is the JAR that includes the modifications in this PR.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

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

@lauriemerrell
Copy link

lauriemerrell commented Feb 3, 2023

Thanks @isabelle-dr! TLDR: I do think that this PR would solve the problem.

Longer explanation of what we're actually doing: in our pipeline (separate from the web validator work), we are trying to join notices data in the warehouse based on the code names that are scraped from RULES.md to get the longer descriptions (rather than automatically generating the link as described here, though the principle is similar). So we have a table with rows that correspond to validator notices, and they are keyed on the values that come from the validator (feed_expiration_date7_days) and another table based on what I scraped from RULES.md which is keyed on how they appear in the markdown (feed_expiration_date_7_days) and the two failed to join.

Copy link
Contributor

@emmambd emmambd left a comment

Choose a reason for hiding this comment

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

Thanks @lauriemerrell for your in depth explanation! It's clearer now that this PR is solving the problem of notices matching.

@isabelle-dr
Copy link
Contributor Author

isabelle-dr commented Feb 3, 2023

Thank you @lauriemerrell, merging this PR then.
We saw the second issue you opened as well. I think it would be good to discuss it in the contributor meeting we have next Monday. The pain from manually updating RULES.md has been on the table for a long time and we've started to think of how to modify the validator to generate this info automatically.

I am sending you an invite for the contributor meeting, hope you can make it!

@fredericsimard fredericsimard merged commit 3d92fa1 into master Feb 3, 2023
@fredericsimard fredericsimard deleted the issue-1322 branch February 3, 2023 15:47
KClough pushed a commit to JarvusInnovations/gtfs-validator that referenced this pull request Feb 24, 2023
* fix RULES.md

* empty commit to trigger tests

* revert commit
KClough pushed a commit to JarvusInnovations/gtfs-validator that referenced this pull request Mar 1, 2023
* fix RULES.md

* empty commit to trigger tests

* revert commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants