Skip to content
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

tool_writeout: refactor write-out and write-out json #6544

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Jan 29, 2021

  • Deduplicate the logic used by write-out and write-out json.

Rather than have separate writeLong, writeString, etc, logic for
each of write-out and write-out json instead have respective shared
functions that can output either format and a 'use_json' parameter to
indicate whether it is json that is output.

This will make it easier to maintain. Rather than have to go through
two sets of logic now we only have to go through one.

  • Support write-out %{errormsg} and %{exitcode} in json.

  • Clarify in the doc that %{exitcode} is the exit code of the transfer.

Prior to this change it just said "The numerical exitcode" which
implies it's the exit code of the tool, and it's not necessarily that.

Closes #xxxx


Note for json this explicitly returns null for keys when the data is not available, which prior to this change was only done for the filename_effective key. That means errormsg would be null if there was no error. Example:

> curld -fsS -o NUL --write-out "%{json}" google.com | jq
{
  "content_type": "text/html; charset=UTF-8",
  "filename_effective": "NUL",
  "exitcode": 0,
  "errormsg": null,
  "ftp_entry_path": null,
  "http_code": 301,
  "http_connect": 0,
  "http_version": "1.1",
  "local_ip": "192.168.1.80",
  "local_port": 58138,
  "method": "GET",
  "num_connects": 1,
  "num_headers": 9,
  "num_redirects": 0,
  "proxy_ssl_verify_result": 0,
  "redirect_url": "http://www.google.com/",
  "remote_ip": "172.217.12.174",
  "remote_port": 80,
  "response_code": 301,
  "scheme": "HTTP",
  "size_download": 219,
  "size_header": 309,
  "size_request": 78,
  "size_upload": 0,
  "speed_download": 3000,
  "speed_upload": 0,
  "ssl_verify_result": 0,
  "time_appconnect": 0,
  "time_connect": 0.038842,
  "time_namelookup": 0.015506,
  "time_pretransfer": 0.038997,
  "time_redirect": 0,
  "time_starttransfer": 0.073571,
  "time_total": 0.073813,
  "url": "google.com",
  "url_effective": "http://google.com/",
  "curl_version": "libcurl/7.75.0-DEV OpenSSL/1.1.1i (Schannel)"
}

@jay jay added cmdline tool feature-window A merge of this requires an open feature window tidy-up labels Jan 29, 2021
@lgtm-com
Copy link

lgtm-com bot commented Jan 29, 2021

This pull request introduces 1 alert when merging 0969181 into 36ef648 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Jan 29, 2021

This pull request introduces 1 alert when merging dd887a0 into 36ef648 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Jan 29, 2021

This pull request introduces 1 alert when merging 8689080 into 36ef648 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@jay jay force-pushed the refactor_writeout branch 2 times, most recently from 30105e7 to 77b164f Compare January 31, 2021 23:02
@ghost
Copy link

ghost commented Jan 31, 2021

Congratulations 🎉. DeepCode analyzed your code in 8.949 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2021

This pull request introduces 1 alert when merging 77b164f into 796ce29 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2021

This pull request introduces 1 alert when merging 1948ecf into 796ce29 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@jay
Copy link
Member Author

jay commented Jan 31, 2021

  • 1 for Comparison result is always the same

LGTM says

if((unsigned long)per->urlnum <= LONG_MAX) {
--
  | NEWComparison is always true because urlnum <= 4294967295.

Some compilers warned about an earlier iteration of this, if(per->urlnum <= LONG_MAX) {. per->urlnum is an unsigned int, but in an attempt to get around that warning I cast to unsigned long. That seems to have worked with some of the compilers but not LGTM. The check is necessary before casting to long (longinfo = (long)per->urlnum;) because in Windows UINT_MAX is 4294967295 and LONG_MAX is 2147483647. I don't see any way to contact them on their website.

- Deduplicate the logic used by write-out and write-out json.

Rather than have separate writeLong, writeString, etc, logic for
each of write-out and write-out json instead have respective shared
functions that can output either format and a 'use_json' parameter to
indicate whether it is json that is output.

This will make it easier to maintain. Rather than have to go through
two sets of logic now we only have to go through one.

- Support write-out %{errormsg} and %{exitcode} in json.

- Clarify in the doc that %{exitcode} is the exit code of the transfer.

Prior to this change it just said "The numerical exitcode" which
implies it's the exit code of the tool, and it's not necessarily that.

Closes #xxxx
@jay jay closed this in 65ca229 Feb 9, 2021
@jay jay removed the feature-window A merge of this requires an open feature window label Feb 9, 2021
@jay jay deleted the refactor_writeout branch February 9, 2021 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

1 participant