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: Update RULES.md to include NOTICES.md information & delete NOTICES.md #1132

Merged
merged 43 commits into from
May 18, 2022

Conversation

isabelle-dr
Copy link
Contributor

@isabelle-dr isabelle-dr commented May 3, 2022

Closes #1006
Closes #1154

Summary:
Following the discussions in PR #1127 (comment), the objective of this PR is to provide a better user experience when reading the validation report. This is part of the effort of the 3.1.0 release.
The HTML report in #1127 will link to the corresponding piece of documentation using the anchor links, in order to ease the process of (1) understanding what the issue is, and (2) fixing the dataset.

This PR contains the following changes:

  • the first part of RULES.md has been reworked to make it more useful for a regular user of the validator. The parts that are useful only when contributing have been moved to NEW_RULES.md, which describes how to add a new rule.
  • the information available in NOTICES.md has been added to RULES.md using a collapsable feature to make the file more digestible. The information added is:
    • the notice_codes
    • the notice fields description
    • the affected files
  • added the notice code into the first table
  • some cleanup & small documentation fixes
  • NEW_RULES.md has been updated to reflect the change of documentation

Expected outcomes:

  • All the documentation that a user needs to understand the validation results & fix their dataset is available in RULES.md
  • The user doesn't need to guess the mapping between the CamelCase NoticeName and the snake_case notice_code

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)

NEW_RULES.md is used if someone wants to contribute to a new rule, and RULES.md is used to interpret the validation report
@isabelle-dr isabelle-dr changed the title Update RULES.md to include NOTICES.md information Update RULES.md to include NOTICES.md information & delete the file May 3, 2022
@isabelle-dr isabelle-dr changed the title Update RULES.md to include NOTICES.md information & delete the file Update RULES.md to include NOTICES.md information & delete NOTICES.md May 3, 2022
@isabelle-dr isabelle-dr changed the title Update RULES.md to include NOTICES.md information & delete NOTICES.md Docs: Update RULES.md to include NOTICES.md information & delete NOTICES.md May 4, 2022
@isabelle-dr isabelle-dr changed the title Docs: Update RULES.md to include NOTICES.md information & delete NOTICES.md docs: Update RULES.md to include NOTICES.md information & delete NOTICES.md May 4, 2022
@isabelle-dr
Copy link
Contributor Author

@maximearmstrong this is ready for review! :)
Do you know why the tests didn't run on this PR?

@maximearmstrong maximearmstrong added this to In Review in The Tech Dashboard (archived) via automation May 4, 2022
@maximearmstrong maximearmstrong marked this pull request as draft May 4, 2022 14:53
@maximearmstrong maximearmstrong marked this pull request as ready for review May 4, 2022 14:53
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.

Some suggestions for changes in-line before approval. Thank you for this @isabelle-dr!

docs/NEW_RULES.md Show resolved Hide resolved
docs/NEW_RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
The Tech Dashboard (archived) automation moved this from In Review to Pending approval May 4, 2022
isabelle-dr and others added 9 commits May 4, 2022 18:04
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
@isabelle-dr
Copy link
Contributor Author

I have applied the changes:

  • remove notice class to use only notice code
  • put the references to the spec outside of the dropdown & add the reference to the spec for notices that were only referring to the python validator
  • other small fixes & typos

@barbeau PTAL, the work is done on my end :)

Copy link
Member

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

Thanks for all this work @isabelle-dr! I think it looks much better than before - much easier to read IMHO.

I found a few small typos in the below comments/suggestions.

docs/NEW_RULES.md Outdated Show resolved Hide resolved
docs/NEW_RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
isabelle-dr and others added 6 commits May 17, 2022 15:24
Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
Co-authored-by: Sean Barbeau <sjbarbeau@gmail.com>
@isabelle-dr
Copy link
Contributor Author

What an eye @barbeau! 🧐 Thank you

@maximearmstrong, could you have a last look & approve this PR if it all looks good?

RULES.md Outdated Show resolved Hide resolved
Copy link
Member

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @isabelle-dr!

The Tech Dashboard (archived) automation moved this from Pending approval to Approved May 17, 2022
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.

2 small things aside, LGTM! Thank you @isabelle-dr :)

Also, we had to reconfigure the CLA assistant last week and I think we will need your signature in order to merge.

RULES.md Outdated Show resolved Hide resolved
RULES.md Outdated Show resolved Hide resolved
isabelle-dr and others added 2 commits May 18, 2022 09:09
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
@maximearmstrong
Copy link
Contributor

Thank you @isabelle-dr ! The other checks won't be performed because this PR only changes the .md files, so I'm merging it.

@maximearmstrong maximearmstrong merged commit 46c46f8 into master May 18, 2022
The Tech Dashboard (archived) automation moved this from Approved to Done May 18, 2022
@maximearmstrong maximearmstrong deleted the documentation-update branch May 18, 2022 14:00
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
4 participants