-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add docstring checker in Workflow #518
Conversation
Do we have any tool to auto generate the template of the docstring? |
shoud work now, we should also define a threshold on how much coverage improvement could be defined as "passed", currently as long as the ci would pass as long as there is any improvment |
I think that’s not the case. We should require the doctoring coverage to be non-decreasing. Instead of asking for improvement in each PR.
Best,
Zhenghao
… On Oct 19, 2023, at 7:26 PM, Zhizheng Liu ***@***.***> wrote:
shoud work now, we should also define a threshold on how much coverage improvement could be defined as "passed", currently as long as the ci would pass as long as there is any improvment
—
Reply to this email directly, view it on GitHub <#518 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFJNUE7AST6YNVB2CL4I52DYAHOLVAVCNFSM6AAAAAA6H7ZBZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZRHE3TMNBWGQ>.
You are receiving this because your review was requested.
|
I totally agree, but isn't the goal of this PR to encourage the coverage little by little after each PR? We could also set a minimum coverage and if the final coverage is greater than this we won't check how the coverage changes from the main branch |
I think current one is more practical like a real CI test. The one you are saying like a little game, which might not be suitable to written into the CI workflow. We can just having several PR to increase the coverage while this might not be reflected as a pass flag in CI. After all the coverage improvement is just a short-term campaign |
Yes, that's like an incentive to encourage more docstrings. |
message |
that's cool stuffs lol |
${message} |
Docstring coverage check successful! You have changed the coverage from 26.832618025751074 to 26.832618025751074 by 0. |
Docstring coverage check successful! You have changed the coverage from 0.000% to 0.000% by 0%. |
You have changed the docstring coverage from 26.833% to 26.833% by 0%. |
[BOT] Great job! You have changed the docstring coverage from 26.833% to 26.833% by 0%. |
What changes do you make in this PR?
Add docstring checker for metadrive
Probably we could merge this PR after all the docstrings are implemented
Checklist
bash scripts/format.sh
before merging.