🔧 Added class for parsing command line parameters #941

AnotherFoxGuy · 2022-05-19T08:13:19Z

Fixes #940

SimplyLiz

Great work! Thank you!

Bogi79 · 2022-05-20T20:43:16Z

src/util/ParseCli.cxx

+    if (args.LastError() != SO_SUCCESS)
+    {
+      LOG(LOG_ERROR) << GetLastErrorText(args.LastError()) << " " << args.OptionText();
+      success = false;


Should it return false immediately?

Bogi79 · 2022-05-20T20:47:26Z

I think it is better not to write to the setting with cmd line parameters.
I would expect it only override current settings but preserve them.

AnotherFoxGuy · 2022-05-26T12:21:37Z

I think it is better not to write to the setting with cmd line parameters.

It doesn't get written to the json file

Bogi79 · 2022-06-01T19:25:09Z

I think it is better not to write to the setting with cmd line parameters.

It doesn't get written to the json file

You have right.

Bogi79 · 2022-06-01T19:39:27Z

src/engine/basics/Settings.hxx

+
+  // ==================================
+  // Command line options
+  // ==================================


Can you extend the comment, that these settings are special ones and will not be preserved in between runs?

dalerank · 2022-06-01T20:48:43Z

src/Game.cxx


-  if (SDL_VideoInit(videoDriver) != 0)
+  if (SDL_VideoInit(vd) != 0)


vd is bad name for variable

dalerank · 2022-06-01T20:49:57Z

src/engine/basics/Settings.hxx

+  // ==================================
+
+  /// Sets a different video driver
+  std::string videoDriver = "Default";


do you really need save it all game? it just option on start, dont pay for unused variables

dalerank · 2022-06-01T20:50:19Z

src/main.cxx

-  {
-    videoDriver = argv[videoOpt + 1];
-  }
+  ParseCli cli;


parser cli?

dalerank · 2022-06-01T20:51:24Z

src/main.cxx

    return EXIT_FAILURE;

-  if (!skipMenu)
+  bool quitGame = Settings::instance().quitGame;


again temporary variable moved to persistant class? did we really need hold this all game?

dalerank · 2022-06-01T20:51:43Z

src/main.cxx

-  if (!skipMenu)
+  bool quitGame = Settings::instance().quitGame;
+
+  if (!Settings::instance().skipMenu)


again temp var in persisten place

dalerank · 2022-06-01T20:56:55Z

src/util/ParseCli.hxx

+#include "SimpleOpt.h"
+#include <string>
+
+class ParseCli


dalerank · 2022-06-01T20:59:45Z

src/util/ParseCli.cxx

+// option identifiers
+enum
+{
+  OPT_HELP,


better user bane enum opt_id {
e_opt_help
....
}
OPT_HELP looks like macros

dalerank · 2022-06-01T21:01:10Z

src/Game.cxx

 {
  if (SDL_Init(0) != 0)
  {
    LOG(LOG_ERROR) << "Failed to Init SDL";
    LOG(LOG_ERROR) << "SDL Error: " << SDL_GetError();
    return false;
  }
+  const char *vd = nullptr;
+  if(Settings::instance().videoDriver != "Default")


space after if

dalerank · 2022-06-01T21:02:06Z

src/Game.hxx


  /** @brief begins the game
    * starts running the game
    * @param SkipMenu if the main menu should be skipped or not
    */
-  virtual void run(bool SkipMenu = false);
+  virtual void run();


why it virtual, we planed have same modes in game?

I don't think that was changed in this PR/commits.

dalerank · 2022-06-01T21:04:16Z

src/main.cxx

-  }
+  ParseCli cli;
+  if (!cli.ProcessCommandLine(argc,argv))
+    return EXIT_FAILURE;

  LOG(LOG_DEBUG) << "Launching Cytopia";

  Cytopia::Game game;

  LOG(LOG_DEBUG) << "Initializing Cytopia";


too much words? debug () << text, debug( "%s", text )?

sonarcloud · 2022-06-02T09:53:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

0.0% Coverage
0.0% Duplication

dalerank · 2022-07-04T20:24:57Z

@AnotherFoxGuy pls fix conflicts and nits

sonarcloud · 2022-07-09T13:13:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

0.0% Coverage
0.0% Duplication

lizzyd710 · 2022-07-10T00:18:32Z

src/util/ParseCli.hxx

+  bool ProcessCommandLine(int argc, char **argv);
+
+private:
+  std::string GetLastErrorText(int a_nError);


Because SonarCloud has infiltrated my head: I think this could be made const.

lizzyd710

Just had some questions about stuff like the video driver parameter, but I may have missed a discussion about that.

lizzyd710 · 2022-07-10T00:20:04Z

src/util/ParseCli.cxx

+#include "Settings.hxx"
+
+// option identifiers
+enum


Does this enum have a name?

lizzyd710 · 2022-07-10T00:23:27Z

src/engine/basics/Settings.hxx

+  // ==================================
+
+  /// Sets a different video driver
+  std::string videoDriver = "Default";


Will video driver handling be added later in a different PR? I guess I'm also just wondering why this parameter is necessary, like I don't know how many people use different drivers for different programs. But then again, I don't normally run Cytopia from the command line.

dalerank · 2022-07-20T08:16:35Z

Pls fix issues and will add this to repo

AnotherFoxGuy requested review from SimplyLiz and lizzyd710 May 19, 2022 08:13

AnotherFoxGuy force-pushed the cli branch from 16e58c3 to 0897002 Compare May 19, 2022 08:13

AnotherFoxGuy requested a review from Bogi79 May 19, 2022 22:14

SimplyLiz previously approved these changes May 20, 2022

View reviewed changes

Bogi79 reviewed May 20, 2022

View reviewed changes

AnotherFoxGuy dismissed SimplyLiz’s stale review via 1cfe8e1 May 26, 2022 12:15

AnotherFoxGuy force-pushed the cli branch from 0897002 to 1cfe8e1 Compare May 26, 2022 12:15

Bogi79 reviewed Jun 1, 2022

View reviewed changes

Bogi79 mentioned this pull request Jun 1, 2022

options parser: add simple options parser, base on artistic style parser #970

Closed

dalerank suggested changes Jun 1, 2022

View reviewed changes

AnotherFoxGuy force-pushed the cli branch from 1cfe8e1 to 0152f7b Compare June 2, 2022 09:41

🔧 Added class for parsing command line parameters

98db48b

AnotherFoxGuy force-pushed the cli branch from 0152f7b to 98db48b Compare July 9, 2022 13:02

lizzyd710 reviewed Jul 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔧 Added class for parsing command line parameters #941

🔧 Added class for parsing command line parameters #941

AnotherFoxGuy commented May 19, 2022

SimplyLiz left a comment

Bogi79 May 20, 2022

Bogi79 commented May 20, 2022

AnotherFoxGuy commented May 26, 2022

Bogi79 commented Jun 1, 2022

Bogi79 Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

dalerank Jun 1, 2022

lizzyd710 Jul 10, 2022

dalerank Jun 1, 2022

sonarcloud bot commented Jun 2, 2022

dalerank commented Jul 4, 2022

sonarcloud bot commented Jul 9, 2022

lizzyd710 Jul 10, 2022

lizzyd710 left a comment

lizzyd710 Jul 10, 2022

lizzyd710 Jul 10, 2022

dalerank commented Jul 20, 2022


		if (SDL_VideoInit(videoDriver) != 0)
		if (SDL_VideoInit(vd) != 0)

🔧 Added class for parsing command line parameters #941

Are you sure you want to change the base?

🔧 Added class for parsing command line parameters #941

Conversation

AnotherFoxGuy commented May 19, 2022

SimplyLiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bogi79 commented May 20, 2022

AnotherFoxGuy commented May 26, 2022

Bogi79 commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 2, 2022

dalerank commented Jul 4, 2022

sonarcloud bot commented Jul 9, 2022

Choose a reason for hiding this comment

lizzyd710 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalerank commented Jul 20, 2022