Zandronum Chat on our Discord Server Get the latest version: 3.1
Source Code
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003249Doomseeker[All Projects] Suggestionpublic2017-09-01 18:062018-10-27 22:55
ReporterWubTheCaptain 
Assigned ToPol M 
PrioritynormalSeveritytweakReproducibilityalways
StatusclosedResolutionfixed 
Platformx86_64OSDebian GNU/LinuxOS Versionbuster/sid
Product Version1.1 
Target Version1.2Fixed in Version1.2 
Summary0003249: Change Doomseeker to fail on invalid command line option
DescriptionSay 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 InformationBehavior 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)

Attached Files

- Relationships

-  Notes
User avatar (0018624)
WubTheCaptain (reporter)
2017-10-25 00:48

 Changed from a bug into a suggestion.
User avatar (0018625)
WubTheCaptain (reporter)
2017-10-25 01:00
edited on: 2017-10-25 01:00

 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.
User avatar (0019312)
Pol M (developer)
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 [^]')
User avatar (0019324)
Zalewa (developer)
2018-08-12 11:45

 'https://bitbucket.org/Doomseeker/doomseeker/commits/fb5c8efef13ccf43e6daec260711e20b04d32f3a [^]'
User avatar (0019340)
WubTheCaptain (reporter)
2018-08-20 18:13

 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.
User avatar (0019346)
Pol M (developer)
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.
User avatar (0019601)
Pol M (developer)
2018-09-21 20:55

 pull request
User avatar (0019604)
WubTheCaptain (reporter)
2018-09-22 01:09

 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?
User avatar (0019605)
WubTheCaptain (reporter)
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.)
User avatar (0019606)
WubTheCaptain (reporter)
2018-09-22 01:16
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.
User avatar (0019621)
Zalewa (developer)
2018-09-22 06:41
Quote from "WubTheCaptain"

fix typos... "doomseeker" being lowercase?

Nah, that's fine.
User avatar (0019622)
Pol M (developer)
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 :)
User avatar (0019623)
Pol M (developer)
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.
User avatar (0019624)
Pol M (developer)
2018-09-22 09:00

 If WubTheCaptain is okay with the changes, I'll "resolve" this ticket.
User avatar (0019625)
Zalewa (developer)
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.
User avatar (0019626)
Pol M (developer)
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)
User avatar (0019627)
Zalewa (developer)
2018-09-22 09:53
edited on: 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.
User avatar (0019628)
Pol M (developer)
2018-09-22 09:54

 Oh, I'll test this 🤔
User avatar (0019631)
WubTheCaptain (reporter)
2018-09-22 13:23
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.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- 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 View Revisions
2017-10-25 01:00 WubTheCaptain Note Added: 0018625
2017-10-25 01:00 WubTheCaptain Note Edited: 0018625 View Revisions
2018-08-05 23:01 Pol M Note Added: 0019312
2018-08-05 23:15 Pol M Note Edited: 0019312 View Revisions
2018-08-05 23:27 Pol M Note Edited: 0019312 View Revisions
2018-08-06 09:13 Pol M Note Edited: 0019312 View Revisions
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 View Revisions
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 View Revisions
2018-09-22 01:13 WubTheCaptain Note Edited: 0019605 View Revisions
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 View Revisions
2018-09-22 09:53 Zalewa Note Added: 0019627
2018-09-22 09:53 Zalewa Note Edited: 0019627 View Revisions
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




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2024 MantisBT Team
Powered by Mantis Bugtracker