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

Depreacte --output-format, only allow --output option #2591

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

Sea-n
Copy link
Contributor

@Sea-n Sea-n commented Dec 9, 2023

Current situation

  • The help message says "-o --output=file.html|json|csv - Output either an HTML, JSON or a CSV file."
  • While using -o (and the deprecated --output-format), it will trigger read_option_args: case 'o'
    • This one checks valid_output_type first, then stores it to output_formats.
  • When using --output, it will trigger parse_long_opt: "output"
    • This one stores it to output_formats directly.

The bug I encountered

While using --output out.tsv, it runs through the parsing process and has no output file.
When changed to -o out.tsv for short, it finally gives me the "invalid filename extension" error.

Refer to commit history

  • In 2016 Jun 2e7c4b8, Added an --output=file.[html|csv|json] as a shortcut to --output-format
  • In 2017 Apr 91da175, Updated README.md with Docker details
    • Changed output-format to output in README.
  • In 2017 Jun 6309ed6, Fix error in example config file
    • The latest --output-format in this repo (except src/options.c) is removed.
  • In 2017 Oct 27d8782, Add valid_output_type() to validate the filename given via -o
    • The file extension check is only applied for -o (--output-format), and the PR author forgot about --output.

According to the commit history, we can see that --output-format has been replaced for 6+ years. I think it's safe to remove it without any warning.

So after this PR is merged, --output-format will only output a fatal error, saying it's deprecated, without backward compatibility.

src/options.c Outdated
Comment on lines 266 to 267
" -o --output=file.html|json|csv - Output either an HTML, JSON or a CSV file.\n"
" -o --output=<filename|format> - Output to stdout or the specificed file.\n"
" Currently support HTML, JSON and CSV format.\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update:
Now I know -o file.csv will save the output to a file, while -o csv will directly display it in the terminal.
It's currently not included in the help message. 😅

I need your assistance in improving the proposed wording, thanks. 🙇‍♂️

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can include a third line to show both options, and a brief example, like the one you posted above, would suffice. Thanks!

@Sea-n
Copy link
Contributor Author

Sea-n commented Dec 13, 2023

I still not confident about how to write the text with short word.

Here is some version (with inspiration of ChatGPT), which one would you preferred?

(A)
-o --output=<format|filename>   - Output to stdout or the specificed file.
                                  Currently support HTML, JSON and CSV format.
                                                                               *
(B)                                                                            *
-o --output=<format|filename>   - Specify the output format or output filename.
                                  The filename must have .html, .json, or .csv extension.
                                                                               *
(C)                                                                            *
-o --output=<html|json|csv>     - Specify the output format.
-o --output=<FILENAME>          - Save report to a file. The FILENAME must
                                  have .html, .json, or .csv extension.
                                                                               *
(D)                                                                            *
-o --output=[FILE.]<FORMAT>     - Output to stdout or the specificed file.
                                  Supported FORMAT: html, json, csv.
                                                                               *
(E)                                                                            *
-o --output=<html|json|csv|FILE> - Set output format or write to a file.
                                   The FILE must have .html, .json, or .csv extension.
                                                                               *
(F)                                                                            *
-o --output=[file.]<html|json|csv> - Output to stdout or the specificed file.

                                                                               *
(G)                                                                            *
-o --output=<format|filename>   - Output to stdout or the specificed file.
                                  e.g., --output=report.html, -o out.json, --output=csv
                                                                               *
  (Current longest text)                                                       *
--unknowns-as-crawlers          - Classify unknown OS and browsers as crawlers.
--external-assets               - Output HTML assets to external JS/CSS files.

@allinurl
Copy link
Owner

Thanks for posting these options. Let's roll with G; I think that's pretty clear, in my opinion.

@Sea-n
Copy link
Contributor Author

Sea-n commented Dec 13, 2023

Okay! Just pushed a new version. ☺️
image

And I do a little adjustment here, think it would be more readable.

(From this)
-o --output=<format|filename>   - Output to stdout or the specified file.
                                  e.g., --output=report.html, -o out.json, --output=csv

(Changed to this)
-o --output=<format|filename>   - Output to stdout or the specified file.
                                  e.g., -o csv, -o out.json, --output=report.html

@allinurl allinurl merged commit 52bd094 into allinurl:master Dec 13, 2023
5 checks passed
@allinurl
Copy link
Owner

Thanks, Sean! The changes have been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants