Conversation
Codecov Report
@@ Coverage Diff @@
## master #33 +/- ##
==========================================
+ Coverage 77.03% 77.70% +0.67%
==========================================
Files 20 20
Lines 627 628 +1
==========================================
+ Hits 483 488 +5
+ Misses 144 140 -4
Continue to review full report at Codecov.
|
|
Awesome! I’ll make some time to review this week. |
| self.white_listed_files = white_listed_files or [] | ||
| self.include_patterns = include_patterns or [] | ||
| self.print_all = print_all | ||
| self.print_level = print_level |
There was a problem hiding this comment.
For all the places that print_level is set from a client, we have to assume that a user might set it to None, thinking this is default or "unset." So instead of just blindly setting it like this, let's do something like:
self.print_level = print_level or "none"
That way if it's set to an empty string or None, it will not break the client.
There was a problem hiding this comment.
But since we favor verbosity, the default should be:
self.print_level = print_level or "all"There was a problem hiding this comment.
makes sense. I will make the change.
|
|
||
|
|
||
| def check_response_status_code(url, response): | ||
| def check_response_status_code(url, response, print_level): |
There was a problem hiding this comment.
The default should be set here, to "all" I think?
|
|
||
| def test_check_response_status_code(): | ||
| @pytest.mark.parametrize( | ||
| "print_level", ["all", "files-with-urls-only", |
There was a problem hiding this comment.
How is files-with-urls-only honored? I don't see it in the printing function, only the other four.
There was a problem hiding this comment.
so files-with-urls-only is not explicitly specified in the code, because I did not need to. Covering the other cases was sufficient. So -as you already noticed- you barely see the variable in the code, but if used, it works as expected.
There was a problem hiding this comment.
This doesn’t make any sense. Please provide an explicit example. If an option isn’t used I don’t understand why it exists.
| # Case 1: response is None indicating triggered error | ||
| if not response: | ||
| print_failure(url) | ||
| if print_level not in ["success-only", "none"] : |
There was a problem hiding this comment.
What about files-with-urls-only?
There was a problem hiding this comment.
I don't need it here, because I want to show failures when files-with-urls-only is specified.
| ): | ||
| self.file_name = file_name | ||
| self.print_all = print_all | ||
| self.print_level = print_level |
There was a problem hiding this comment.
Especially if print_level is default to None here, setting the variable again needs to be:
self.print_level = print_level or "all"| # if no urls are found, mention it if required | ||
| if not urls: | ||
| if self.print_all: | ||
| if self.print_level == "all": |
There was a problem hiding this comment.
I don't think this should be subject to print level - this message should be printed to tell the user there are no urls regardless. I would remove this if statement here and restore to how it was.
There was a problem hiding this comment.
Unfortunately I disagree, I totally get your point but I think merging all printing into one option with multiple choices is much better than having a flag and printing variable which can be confusing in my opinion :/
There was a problem hiding this comment.
The user needs to know if there are not any urls, which is different from just not printing results. If you want to control verbosity of non url messages then we can discuss adding a logger (also with levels) but shoving all printing under one variable because it’s easy is not appropriate.
There was a problem hiding this comment.
You got a point but It is not about easy or hard, the changes are not major. I just have the feeling that we are having a lot of input variables at this point, which is nice but it can be confusing to users.
|
I think I’ve changed my mind about this feature - given the core use of url checker for CI runs, it feels like feature bloat, and I’m not convinced that this is the cleanest implementation to insert a bunch of random if/else all over the library. Implementing this with a logger type interface that calls a statement based on a level would be much more elegant. I think the work is still valuable and we should keep the PR for future feedback but I am not wanting to add this now to the library. Thanks for your work on this @SuperKogito though! |
|
okay xD what made this sudden change of heart? |
2 participants
--print-levelfeature and removes the old--print-allflag.--print-levelhas 4 printing levels:all: all printsfiles-with-urls-only: print only for files with urls in them.fails-only: print only broken links.success-only: print only working links.none: no files or urls prints.