docs: clarify that make ensure-generated is a PR requirement#513
docs: clarify that make ensure-generated is a PR requirement#513jackfrancis merged 3 commits intoAzure:masterfrom
make ensure-generated is a PR requirement#513Conversation
| ## Pull Requests and Generated Code | ||
|
|
||
| **Summary:** Always run `make ensure-generated` before you submit a pull request. If that command | ||
| returns an error, it will also have generated files that need to be included with the PR. |
There was a problem hiding this comment.
Why would ensure-generated return an error if it also resulted in the needed generated files? That sounds like something that would return 0.
There was a problem hiding this comment.
Because if the command fails, the PR author has more to do (commit the files and try again). So it returns an error return code.
Since it had to generate the files in order to know if there's a diff, the simplest path is to have the user just commit the files already generated. Committing what could be seen as a side effect does seem odd, which was the motivation for renaming the make target to ensure-generated.
Codecov Report
@@ Coverage Diff @@
## master #513 +/- ##
=======================================
Coverage 54.39% 54.39%
=======================================
Files 97 97
Lines 14570 14570
=======================================
Hits 7925 7925
Misses 5971 5971
Partials 674 674 |
|
Thanks so much for this @mboersma. Can we add an extra step in the PR template to highlight this requirement? |
| displayName: Run linting rules | ||
| workingDirectory: $(modulePath) | ||
| - script: make validate-generated | ||
| - script: make ensure-generated |
Yes indeed. I will add that. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, mboersma The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Reason for Change:
#449 failed to document that keeping generated files in sync is now a requirement for PRs submitted.
Issue Fixed:
Fixes #509
Requirements:
Notes:
This PR repeats some content so it's less likely to be missed. The root CONTRIBUTING.md document isn't really unified with those under
docs/, so I allowed the repetition and linked from one to the other.The script is already fairly explicit about what you need to do when there are diffs, so I didn't add any more exhortation in the Makefile: