-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
audit: specify which URL has a content problem in problem message #11203
Conversation
Thanks for looking into this! What about if the stable URL is a |
As in a private GitHub (for example) repo? In theory it should have the same error message as it used to, with the first instance of “URL” replaced with “zip/tarball URL”. I can’t check this right now, but I’ll see if I can verify that when I’m back to my computer tomorrow. |
Oh, I just meant that this message would also call Git repo URLs a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Library/Homebrew/formula_auditor.rb
Outdated
@@ -383,7 +383,7 @@ def audit_homepage | |||
user_agents: [:browser, :default], | |||
check_content: true, | |||
strict: @strict)) | |||
problem http_content_problem | |||
problem http_content_problem.sub("URL", "homepage URL") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally homepage URL
would be an argument to curl_check_http_content
rather than substituting the output at the end. It would be good to make this a required argument and also use it in Library/Homebrew/cask/audit.rb
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! I’ll also try to look into what I can do for this tonight.
Ohh, sorry, I completely missed the point! Yeah, I didn’t account for that at all. I’ll try to see what I can do about it tonight. |
Force pushed some changes based on your reviews. Please let me know if anything else needs improvement! |
Library/Homebrew/resource_auditor.rb
Outdated
@@ -101,7 +101,7 @@ def audit_urls | |||
|
|||
strategy = DownloadStrategyDetector.detect(url, using) | |||
if strategy <= CurlDownloadStrategy && !url.start_with?("file") | |||
if (http_content_problem = curl_check_http_content(url, specs: specs)) | |||
if (http_content_problem = curl_check_http_content(url, "zip, tarball, or repo URL", specs: specs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (http_content_problem = curl_check_http_content(url, "zip, tarball, or repo URL", specs: specs)) | |
if (http_content_problem = curl_check_http_content(url, "source URL", specs: specs)) |
This seems nicer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you. I thought there was a more concise way to say it, but it was escaping my brain 😛
Change made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good here, nice work!
Thanks again @kthchew! |
You’re welcome! 😊 |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Implements #11183 (CC @cdayjr). The two new messages are (for example):
(formerly did not mention "homepage" or "zip/tarball")