Adding files flag and fixing bug with return code#28
Conversation
Codecov Report
@@ Coverage Diff @@
## master #28 +/- ##
==========================================
- Coverage 72.94% 72.31% -0.63%
==========================================
Files 15 15
Lines 499 513 +14
==========================================
+ Hits 364 371 +7
- Misses 135 142 +7
Continue to review full report at Codecov.
|
|
@SuperKogito please review but let's not address code cov for now, this is an urgent fix because the library and action have a bu. |
|
@SuperKogito did you mean to review this, or just thumbs up? |
SuperKogito
left a comment
SuperKogito
left a comment
There was a problem hiding this comment.
The PR looks good overall, I have one comment to the force pass bug.
The rest looks okay and since they passed the tests it should be fine.
| for failed_url in check_results["failed"]: | ||
| print_failure(failed_url) | ||
| sys.exit(1) | ||
|
|
There was a problem hiding this comment.
This will work fine but the final output is very helpful and the force pass is more about the sys.exit(). Wouldn't it make sense, to keep the print and just change the exit status?
There was a problem hiding this comment.
What do you mean? The print is still there, the change just shows changing the quotation style.
There was a problem hiding this comment.
no I mean in case there is force pass is true, and there are failed links, the urlchecker will force the pass but no urls are printed, which actually helpful to see imo. idk if is that big, might be a matter of preference. My idea is that the force pass is just to exit with 1 and it should not change the prints.
There was a problem hiding this comment.
Oh I understand! Yes let me fix this now. You'll need to approve it again so don't approve it again yet :)
There was a problem hiding this comment.
Yes, I think I might have run it accidentally, but the code is so neat the changes were very minimal!
There was a problem hiding this comment.
I am just asking because I noticed that we use different styles and it might be good for the future to figure out a general standard ;)
There was a problem hiding this comment.
I used to have my own weird style many years ago, and then I was using pylint for a while (and it was quite arduous) and when I noticed snakemake using black, and then tried it myself and thought that it made the code lovely, I started using black. It does have minor changes between versions, but it's so easy to use and add to CI I use it for most of my python projects. Locally I just do:
black <project>
and then in CI you can do:
black --check <project>
There was a problem hiding this comment.
I personally like https://github.com/google/yapf but I also have my hybrid style based on some weird preferences that I am trying to get rid of xD I will take a look at black -which is trending these days- and see how is it.
|
The thumbs up = as in I am reviewing it. The |
|
@SuperKogito the bug was that the force-pass needed a not before it, so the bug is fixed here. What are you referring to specifically? I just pushed a commit that updates the README.md to include the new flag, and fixes a spelling mistake I saw for save so you'll have to approve again :) |
|
I just explained this a bit in the review thread/ comments. |
Signed-off-by: vsoch <vsochat@stanford.edu>
|
|
||
| # Case 1: We didn't find any urls to check | ||
| if not check_results['failed'] and not check_results['passed']: | ||
| if not check_results["failed"] and not check_results["passed"]: |
There was a problem hiding this comment.
@SuperKogito I hope this better matches what you had in mind:
- If there aren't any passing or failing urls, we exit with code 0
- If there are failed urls, regardless of force pass, we print them for the user in red
- Then if there are failed and there is no force pass, we exit with error
- Finally, we exit with 0 (the last condition given that there are either failed urls with force pass, or there were none) but we modify the message printed so the user knows if it was a conditional pass.
|
Woot! Let's get this deployed, I'll open my PR for urlchecker-action asap! |
2 participants
This PR will address the following issues:
There is more work to be done with other issues, but these issues need to be triaged to fix the currently broken library and action.
Signed-off-by: vsoch vsochat@stanford.edu