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

Quality explain #14264

Merged
merged 5 commits into from
Nov 3, 2021
Merged

Quality explain #14264

merged 5 commits into from
Nov 3, 2021

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Nov 3, 2021

What does this PR do?

This PR reorganizes the quality checks by splitting the current make quality in two:

  • make quality is the check on code style
  • make repo-consistency contains all other general checks on the repo

Similarly in the CI checks, the old check_repository_consistency check (that tested links used in the library are on S3 back in the pre-git repo days) performs the checks of make repo-consistency, while check-code-quality only focuses on the code quality.

In practice, nothing changes for the users of make fixup, but users of make quality have to remember two different lines:
make quality and make repo-consistency.

I also have drafted a doc page explaining all those changes and what each check does (for advanced contributors as well as team members), with TODOs of checks I plan to add in the future.

One last future plan I also have is to make the make repo-consistency command perform all checks and report all failures at the end (instead of stopping at the first one).

Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

Looks fantastic, Sylvain. Thank you for adding this important guide.

I'd only suggest to rename the doc to pr_checks (tests already has a different meaning, so it'd be misleading).

Also left a few suggestions to the wording.

docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
docs/source/pr_tests.md Outdated Show resolved Hide resolved
Co-authored-by: Stas Bekman <stas00@users.noreply.github.com>
@sgugger
Copy link
Collaborator Author

sgugger commented Nov 3, 2021

Thanks for the extended review @stas00 ! Will rename the page.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Very nice, thank you for taking care of that @sgugger!

@sgugger sgugger merged commit f0d6e95 into master Nov 3, 2021
@sgugger sgugger deleted the quality_explain branch November 3, 2021 21:43
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 27, 2022
* Start PR doc

* Cleanup the quality checks and document them

* Add reference in the contributing guide

* Apply suggestions from code review

Co-authored-by: Stas Bekman <stas00@users.noreply.github.com>

* Rename file as per review suggestion

Co-authored-by: Stas Bekman <stas00@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants