MantisBT - Doomseeker |
| View Issue Details |
|
| ID | Project | Category | View Status | Date Submitted | Last Update |
| 0003249 | Doomseeker | [All Projects] Suggestion | public | 2017-09-01 18:06 | 2018-10-27 22:55 |
|
| Reporter | WubTheCaptain | |
| Assigned To | Pol M | |
| Priority | normal | Severity | tweak | Reproducibility | always |
| Status | closed | Resolution | fixed | |
| Platform | x86_64 | OS | Debian GNU/Linux | OS Version | buster/sid |
| Product Version | 1.1 | |
| Target Version | 1.2 | Fixed in Version | 1.2 | |
|
| Summary | 0003249: Change Doomseeker to fail on invalid command line option |
| Description | Say I made a typo and instead of "--datadir /foo/bar/" option I omitted a character and had a typo "--datdir /foo/bar", Doomseeker would still start but with wrong (default) IP2C or plugin data I didn't intend to use. There's also a possibility of not understanding what went wrong in the case. |
| Steps To Reproduce | $ doomseeker --foo-bar --this-is-not-a-real-option
[20:48:56] Starting Doomseeker. Hello World! :)
...
(Doomseeker starts normally.) |
| Additional Information | Behavior with unrecognized options varies between programs. Few examples are listed below.
$ cat --foo-bar --this-is-not-a-real-option
cat: unrecognized option '--foo-bar'
Try 'cat --help' for more information.
$ slock --foo-bar --this-is-not-a-real-option
usage: slock [-v] [cmd [arg ...]]
$ gcc --foo-bar --this-is-not-a-real-option
gcc: error: unrecognized command line option ‘--foo-bar’
gcc: error: unrecognized command line option ‘--this-is-not-a-real-option’
gcc: fatal error: no input files
compilation terminated.
$ mpv --foo-bar --this-is-not-a-real-option
Error parsing option foo-bar (option not found)
Setting commandline option --foo-bar= failed.
Exiting... (Fatal error)
§ firefox --foo-bar --this-is-not-a-real-option
[Firefox starts normally.]
To improve user experience, I would suggest Doomseeker to fail with an informative message when a command line option for Doomseeker which doesn't exist is attempted.
- http://man7.org/linux/man-pages/man3/getopt.3.html
- http://pubs.opengroup.org/onlinepubs/9699919799/functions/getopt.html from POSIX.1-2008 (2016)
|
| Tags | No tags attached. |
| Relationships | |
| Attached Files | |
|
| Issue History |
| Date Modified | Username | Field | Change |
| 2017-09-01 18:06 | WubTheCaptain | New Issue | |
| 2017-10-25 00:48 | WubTheCaptain | Category | Bug => Suggestion |
| 2017-10-25 00:48 | WubTheCaptain | Summary | Doomseeker doesn't fail on invalid command line option => Change Doomseeker to fail on invalid command line option |
| 2017-10-25 00:48 | WubTheCaptain | Note Added: 0018624 | |
| 2017-10-25 00:56 | WubTheCaptain | Additional Information Updated | bug_revision_view_page.php?rev_id=11194#r11194 |
| 2017-10-25 01:00 | WubTheCaptain | Note Added: 0018625 | |
| 2017-10-25 01:00 | WubTheCaptain | Note Edited: 0018625 | bug_revision_view_page.php?bugnote_id=18625#r11196 |
| 2018-08-05 23:01 | Pol M | Note Added: 0019312 | |
| 2018-08-05 23:15 | Pol M | Note Edited: 0019312 | bug_revision_view_page.php?bugnote_id=19312#r11602 |
| 2018-08-05 23:27 | Pol M | Note Edited: 0019312 | bug_revision_view_page.php?bugnote_id=19312#r11603 |
| 2018-08-06 09:13 | Pol M | Note Edited: 0019312 | bug_revision_view_page.php?bugnote_id=19312#r11604 |
| 2018-08-12 11:45 | Zalewa | Note Added: 0019324 | |
| 2018-08-12 11:45 | Zalewa | Assigned To | => Zalewa |
| 2018-08-12 11:45 | Zalewa | Status | new => needs testing |
| 2018-08-20 18:13 | WubTheCaptain | Note Added: 0019340 | |
| 2018-08-20 18:13 | WubTheCaptain | Status | needs testing => needs review |
| 2018-08-20 19:00 | WubTheCaptain | Target Version | => 1.2 |
| 2018-08-20 19:22 | Pol M | Note Added: 0019346 | |
| 2018-08-21 13:16 | Pol M | Note Edited: 0019346 | bug_revision_view_page.php?bugnote_id=19346#r11654 |
| 2018-08-21 23:47 | Zalewa | Assigned To | Zalewa => Pol M |
| 2018-08-21 23:47 | Zalewa | Status | needs review => assigned |
| 2018-09-21 20:55 | Pol M | Note Added: 0019601 | |
| 2018-09-21 20:55 | Pol M | Status | assigned => needs review |
| 2018-09-22 01:09 | WubTheCaptain | Note Added: 0019604 | |
| 2018-09-22 01:12 | WubTheCaptain | Note Added: 0019605 | |
| 2018-09-22 01:12 | WubTheCaptain | Note Edited: 0019605 | bug_revision_view_page.php?bugnote_id=19605#r11883 |
| 2018-09-22 01:13 | WubTheCaptain | Note Edited: 0019605 | bug_revision_view_page.php?bugnote_id=19605#r11884 |
| 2018-09-22 01:16 | WubTheCaptain | Note Added: 0019606 | |
| 2018-09-22 06:41 | Zalewa | Note Added: 0019621 | |
| 2018-09-22 08:46 | Pol M | Note Added: 0019622 | |
| 2018-09-22 08:59 | Pol M | Note Added: 0019623 | |
| 2018-09-22 09:00 | Pol M | Note Added: 0019624 | |
| 2018-09-22 09:32 | Zalewa | Note Added: 0019625 | |
| 2018-09-22 09:46 | Pol M | Note Added: 0019626 | |
| 2018-09-22 09:48 | Pol M | Note Edited: 0019626 | bug_revision_view_page.php?bugnote_id=19626#r11898 |
| 2018-09-22 09:53 | Zalewa | Note Added: 0019627 | |
| 2018-09-22 09:53 | Zalewa | Note Edited: 0019627 | bug_revision_view_page.php?bugnote_id=19627#r11900 |
| 2018-09-22 09:54 | Pol M | Note Added: 0019628 | |
| 2018-09-22 13:23 | WubTheCaptain | Note Added: 0019631 | |
| 2018-09-22 13:23 | WubTheCaptain | Status | needs review => resolved |
| 2018-09-22 13:23 | WubTheCaptain | Fixed in Version | => 1.2 |
| 2018-09-22 13:23 | WubTheCaptain | Resolution | open => fixed |
| 2018-10-27 22:55 | WubTheCaptain | Status | resolved => closed |
|
Notes |
|
|
|
|
Changed from a bug into a suggestion. |
|
|
|
|
Nevermind me linking additional information to getopt, getopt_long() are GNU extensions but I think the part about "?" (an error) is worth looking more into.
|
|
|
|
(0019312)
|
|
Pol M
|
2018-08-05 23:01
(edited on: 2018-08-06 09:13) |
|
since --rcon is a two-argument option, getopt and getopt_long maybe are not the best way to handle the argv of doomseeker. those cannot handle two-argument options, and the ideas I've found to explicitly use getopt_long seem to require modifications to the way the command is handled, for instance:
doomseeker --rconplugin [plugin] --rconip [ip]
doomseeker --rcon [plugin]=[ip]
doomseeker --rcon "[plugin] [ip]"
I can do what you ask, even without getopt. but if we want to use getopt_long, it looks like it could change the way argv works for the end user. In case that getopt_long can handle multiple arguments and I'm simply horrible at googling, please tell me :)
Also, I think that doomseeker should also stop if an option with an obligatory argument is ran without it. It should also fail.
EDIT: For instance, I've made a fork that would do exactly what you're asking for. (hg clone'https://Pol_M@bitbucket.org/Pol_M/doomseeker-addr-0003249 [^]')
|
|
|
|
(0019324)
|
|
Zalewa
|
|
2018-08-12 11:45
|
|
|
|
|
|
I don't like this if-else soup and code duplication that is being brewed from code quality perspective, but the patch functionality seems to be working fine as of hg changeset 2205:aafacb71daee.
Would you clean up the code? Else, close this bug as resolved. |
|
|
|
(0019346)
|
|
Pol M
|
2018-08-20 19:22
(edited on: 2018-08-21 13:16) |
|
Ok, I'll try to keep it a little bit more tidy with a switch, and I think I can cut a few other lines.
EDIT: Forgot that C++ switch did not work with Strings. Still, I'll try to cut repeated code.
|
|
|
|
(0019601)
|
|
Pol M
|
|
2018-09-21 20:55
|
|
|
|
|
|
I still wish we didn't have this if-else string comparison code, it's nasty; But fair enough. At least the duplicate tr() is gone.
Zalewa requested to fix typos... not sure what that means. "doomseeker" being lowercase? |
|
|
|
(0019605)
|
|
WubTheCaptain
|
2018-09-22 01:12
(edited on: 2018-09-22 01:13) |
|
Quote from WubTheCaptain
Zalewa requested to fix typos... not sure what that means. "doomseeker" being lowercase?
Ah, you had already fixed those typos with commit 26b892cf41c197aa2b6d4c59af2b657ef804048a.
I'm not sure if "doomseeker" should be all lowercase or first letter capitalized. (Or there at all.)
|
|
|
|
|
Quote from Pol M
pull request
Side-note again about pull requests: It'd be easier to find the relevant pull requests if the PR title (currently "Addresses ticket 0003249 from WubTheCaptain") would be at the end of description, and the title descriptively said something like "Cleanup Main::interpretCommandLineParameters()" or similar. |
|
|
|
(0019621)
|
|
Zalewa
|
|
2018-09-22 06:41
|
|
Quote from "WubTheCaptain"
fix typos... "doomseeker" being lowercase?
Nah, that's fine. |
|
|
|
(0019622)
|
|
Pol M
|
|
2018-09-22 08:46
|
|
Quote from WubTheCaptain
Side-note again about pull requests: It'd be easier to find the relevant pull requests if the PR title (currently "Addresses ticket 0003249 from WubTheCaptain") would be at the end of description, and the title descriptively said something like "Cleanup Main::interpretCommandLineParameters()" or similar.
Ok, I'll put a more descriptive title and leave the number of the ticket for the end of the description.
Also, I'd like to add that it seems that bitbucket does not have bookmarks especially well integrated. When I sync with the official repo, It also tries to merge heads, completely ignoring the bookmarks. To solve this, either I strip the merge commit that bitbucket generates (fortunately I'm the only one using my repo, so manipulating history before I pull changes shouldn't be bad.) or I use branches, which are well integrated. For the moment I'll proceed with the first option, but if someone has a better idea, let me know :) |
|
|
|
(0019623)
|
|
Pol M
|
|
2018-09-22 08:59
|
|
Quote from WubTheCaptain
I still wish we didn't have this if-else string comparison code, it's nasty; But fair enough.
I don't like having an if/else chain there, I'd use a switch. But since C++'s switch does not support strings, it would be a little bit difficult. An option would be to transform the strings to numbers through some conversion, or an enum and a map trick. Both options don't really convince me for this purpose. |
|
|
|
(0019624)
|
|
Pol M
|
|
2018-09-22 09:00
|
|
|
If WubTheCaptain is okay with the changes, I'll "resolve" this ticket. |
|
|
|
(0019625)
|
|
Zalewa
|
|
2018-09-22 09:32
|
|
I was able to successfully use bookmarks to contribute to Zandronum, albeit it was a while ago and I don't quite remember the details of operations. IIRC I operated partially in TortoiseHg UI and partially from the command prompt as CMD was too clumsy for some purposes and the GUI did not have all the options.
Quote from "Pol M"
I don't like having an if/else chain there, I'd use a switch. But since C++'s switch does not support strings, it would be a little bit difficult. An option would be to transform the strings to numbers through some conversion, or an enum and a map trick. Both options don't really convince me for this purpose.
I suppose this could be done by using a std::map<string, ArgHandler*> object with ArgHandler having polymorphed implementations to handle each argument, or even avoid using std::map and have an array of ArgHandler with '.arg()' method returning the string to compare to but with the version of C++ we use the code would actually be even worse.
C++11 (or is it 14?) has lambdas, which would improve this ordeal a bit, though I'm also unconvinced that the result would be any better.
Maybe Wub knows of some example that we could look at? A one that preferably doesn't use an external library. |
|
|
|
(0019626)
|
|
Pol M
|
2018-09-22 09:46
(edited on: 2018-09-22 09:48) |
|
Quote from Zalewa
I was able to successfully use bookmarks to contribute to Zandronum, albeit it was a while ago and I don't quite remember the details of operations. IIRC I operated partially in TortoiseHg UI and partially from the command prompt as CMD was too clumsy for some purposes and the GUI did not have all the options.
The error I mention occurs only when I sync my repo with the official repo. I'm unaware if it is a mistake of the way I use bookmarks, but I'll confidently say that this time it is not my fault. :)
Quote from Zalewa
I suppose this could be done by using a std::map<string, ArgHandler*> object with ArgHandler having polymorphed implementations to handle each argument, or even avoid using std::map and have an array of ArgHandler with '.arg()' method returning the string to compare to but with the version of C++ we use the code would actually be even worse.
C++11 (or is it 14?) has lambdas, which would improve this ordeal a bit, though I'm also unconvinced that the result would be any better.
That's what I've thought. :/ (C++11)
|
|
|
|
(0019627)
|
|
Zalewa
|
|
2018-09-22 09:53
|
|
Quote from "Pol M"
The error I mention occurs only when I sync my repo with the official repo. I'm unaware if it is a mistake of the way I use bookmarks, but I'll confidently say that this time it is not my fault. :)
Ah, I see now. I never used the "Sync" button on Bitbucket's webpage. I pulled everything from the official repo to a repository on my local machine and then pushed it back to my repo on the site. It gives you a lot more flexibility in deciding what goes where.
|
|
|
|
(0019628)
|
|
Pol M
|
|
2018-09-22 09:54
|
|
|
|
|
|
Quote from Zalewa
Maybe Wub knows of some example that we could look at? A one that preferably doesn't use an external library.
I won't be spending any more time on resolving the if-else soup. C++ is not my cup of tea. Besides, it's not entirely in scope of this ticket to resolve; Create a child ticket if needed.
Since this has been merged with merge commit 365d5abf3b5a3c10c1dd01114b6d21572c6cb5aa, I assume this ticket is resolved. |
|