:wrench: Added class for parsing command line parameters by AnotherFoxGuy · Pull Request #941 · CytopiaTeam/Cytopia

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

+    vd = Settings::instance().videoDriver.c_str();

-  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

    return false;
  }
+  const char *vd = nullptr;
+  if(Settings::instance().videoDriver != "Default")


space after if

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

src/Game.hxx

    * @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


  Cytopia::Game game;

  LOG(LOG_DEBUG) << "Initializing Cytopia";


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

sonarqubecloud · 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

sonarqubecloud · 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


		Cytopia::Game game;

		LOG(LOG_DEBUG) << "Initializing Cytopia";

Conversation

AnotherFoxGuy commented May 19, 2022

Uh oh!

SimplyLiz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bogi79 commented May 20, 2022

Uh oh!

AnotherFoxGuy commented May 26, 2022

Uh oh!

Bogi79 commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 2, 2022

Uh oh!

dalerank commented Jul 4, 2022

Uh oh!

sonarqubecloud bot commented Jul 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizzyd710 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalerank commented Jul 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants