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

--dry-run option is not working #1637

Closed
mwestphal opened this issue Sep 27, 2024 · 17 comments
Closed

--dry-run option is not working #1637

mwestphal opened this issue Sep 27, 2024 · 17 comments
Assignees
Milestone

Comments

@mwestphal
Copy link
Contributor

Describe the bug
F3D usually uses a config file, but there is an option to disable this behavior, --dry-run. It seems to be non-functionnal currently

To Reproduce
Steps to reproduce the behavior, using a built f3d with BUILD_TESTING enabled:

  1. Open the file using `f3d --config=config_build --dry-run --verbose
  2. F3D appears with axis visible, it should not
  3. Check the output, a config file is read

Expected behavior
Config file should not be read

Additional context
in F3DStarter.cxx:655, although the comment says that dry-run is being checked, its actually not. Add the check here by checking the cli option and position the boolean accordingly. Also add a test for it in application/testing/CMakeLists.txt

@mwestphal mwestphal added this to F3D Sep 27, 2024
@mwestphal mwestphal added this to the 3.0.0 milestone Sep 27, 2024
@mwestphal mwestphal moved this to To do in F3D Sep 27, 2024
@snoyer
Copy link
Contributor

snoyer commented Sep 28, 2024

Would this be the right time to rename this argument to --no-config or something?
--dry-run is used in many CLI applications to mean "run as normal but don't actually write/change anything" so it's a bit confusing (F3D's --no-render is closer to what --dry-run usually means)

@mwestphal
Copy link
Contributor Author

Good point @snoyer

@Yogesh9000
Copy link
Contributor

Is this issue still open? If it is I would like to work on it.

@mwestphal
Copy link
Contributor Author

It is open!

@mwestphal
Copy link
Contributor Author

Hum, since you are working on another issue, I'd like to keep this one open for other developpers with no experience on the project if thats ok

@Yogesh9000
Copy link
Contributor

sure thats fine

@rehanganapathy
Copy link

Hey there, this looks interesting, if no one else is working on it, can i pick it up? Thank you so much!
@mwestphal

@mwestphal
Copy link
Contributor Author

Sounds good! @rehanganapathy , go for it :)

@mwestphal
Copy link
Contributor Author

@rehanganapathy any news on this ?

rkoudsi pushed a commit to rkoudsi/f3d that referenced this issue Nov 4, 2024
for issue f3d-app#1637
this will make it clearer as dry-run does not do what most people
expect (not make changes).
This new argument will lead people to search for no-render if they
actually intend to use the feature that most projects refer to as
dry-run
@mwestphal
Copy link
Contributor Author

no feedback, unassigning it

@t-h2o
Copy link
Contributor

t-h2o commented Nov 16, 2024

Hello!

With this patch, the config value is never set.

 application/F3DStarter.cxx | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/application/F3DStarter.cxx b/application/F3DStarter.cxx
index e1cc563a..6b00c5bc 100644
--- a/application/F3DStarter.cxx
+++ b/application/F3DStarter.cxx
@@ -713,12 +713,16 @@ int F3DStarter::Start(int argc, char** argv)
   // but this duplicate the initialization value as it is present in
   // F3DOptionTools::DefaultAppOptions too
   bool dryRun = false;
+  if (cliOptionsDict.find("dry-run") != cliOptionsDict.end())
+  {
+    dryRun = f3d::options::parse<bool>(cliOptionsDict["dry-run"]);
+  }
   if (cliOptionsDict.find("no-render") != cliOptionsDict.end())
   {
     dryRun = f3d::options::parse<bool>(cliOptionsDict["no-render"]);
   }
   std::string config;
-  if (cliOptionsDict.find("config") != cliOptionsDict.end())
+  if (!dryRun && cliOptionsDict.find("config") != cliOptionsDict.end())
   {
     config = f3d::options::parse<std::string>(cliOptionsDict["config"]);
   }

Is it correct and/or a good starting point?

@mwestphal
Copy link
Contributor Author

It is definitely a good starting point! Do you want to open a PR with this patch @t-h2o ?

@t-h2o t-h2o mentioned this issue Nov 17, 2024
5 tasks
@t-h2o
Copy link
Contributor

t-h2o commented Nov 17, 2024

It is definitely a good starting point! Do you want to open a PR with this patch @t-h2o ?

Done: #1720

@v0id-strike
Copy link

That's me, vincenzo. i am getting this issue

@mwestphal
Copy link
Contributor Author

Sorry, this issue is not available

@t-h2o
Copy link
Contributor

t-h2o commented Dec 17, 2024

That's me, vincenzo. i am getting this issue

You can review the current PR on this issue #1720 if you want

@mwestphal
Copy link
Contributor Author

FIxed by #1720

@github-project-automation github-project-automation bot moved this from To do to Done in F3D Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

6 participants