Don't crash in case of unexpected build date format #178
Conversation
try to parse the date, if unsuccessful, give up instead of crashing entirely
michaelgrifalconi
force-pushed
the
date-parsing
branch
from
ad50b4c to
78d2e74
Compare
|
And https://github.com/openSUSE/qem-bot/actions/runs/8875669610/job/24365587576?pr=178#step:6:19 says "openqabot/approver.py:160:4: R0911: Too many return statements (7/6) (too-many-return-statements)" |
About this, I would vote for disabling the check in this specific case. The reasons of this many returns are to "fail early" instead of going to the end of the function for no reason. But we can argue to still do that. So the options I see are:
I can do whatever is requested, but I would like to find something that still makes sense to me, what is your opinion here? |
| current_build_date = datetime.strptime(current_build, "%Y%m%d") | ||
| except (ValueError, TypeError): | ||
| log.info("Could not parse build date %s", current_build) | ||
| return False |
There was a problem hiding this comment.
Moving your discussion into a thread here for easier handling:
And https://github.com/openSUSE/qem-bot/actions/runs/8875669610/job/24365587576?pr=178#step:6:19 says "openqabot/approver.py:160:4: R0911: Too many return statements (7/6) (too-many-return-statements)"
About this, I would vote for disabling the check in this specific case.
The example provided in pylint docs is for a very different situation that does not apply here: https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/too-many-return-statements.html
I don't think we are generally smarter than the people refining the proper python style over years so I don't think we should disable that check :)
The reasons of this many returns are to "fail early" instead of going to the end of the function for no reason. But we can argue to still do that.
So the options I see are:
- move the for loop into a different function: I don't like this since would be a 'function' only called once from one single place, making it more difficult to understand the flow of the code due to jumping around in the file.
- change return to result=False/True and return only at the end, adding more line of code just to make the check happy
I can do whatever is requested, but I would like to find something that still makes sense to me, what is your opinion here?
My opinion would actually depend on an answer in https://github.com/openSUSE/qem-bot/pull/178/files#r1582721997 so we should resolve that first.
In general I would be ok with both options. Additionally you can also consider using Exceptions instead of returns over multiple levels and reconsider how this function is called in the caller and also how the caller is called in its caller
There was a problem hiding this comment.
I don't think we are generally smarter
Well they offer a way to ignore the check so their "smartness" considered the option to do so :)
I proposed a new log message for both cases (failed parsing on main job and on possible alternative job). Feel free to suggest better wording, I hope what I try to explain is clear!
Then I wait for the proposal to handle the issue and keep the code easy to understand, avoiding more complicated workarounds just to hide a few simple returns because the tool say so.
There was a problem hiding this comment.
Please see my suggestion #180 regarding how to handle the "too many return statements". Feel welcome to incorporate my changes into your PR or ask me to push the according changes with conflict resolution into your branch
There was a problem hiding this comment.
Hello and thanks for the proposal! Wouldn't have made more sense if you submitted a pr for this branch, so your changes could be applied directly instead of having me (once again :P) resolve conflicts on an ongoing PR with the risk of messing something up?
There was a problem hiding this comment.
… or rather: Can do but first I would like to resolve #180 (review) and any other potential subsequent comments
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
Included with #181 |
try to parse the date, if unsuccessful, give up instead of crashing entirely
This is to fix cases like https://gitlab.suse.de/qa-maintenance/bot-ng/-/jobs/2463822
where a manually cloned job with unexpected build strings confuse the bot logic.