Set start of 'ok' and improve check of empty 'file' string

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.

https://doc.modelica.org

BSD 3-Clause "New" or "Revised" License

452 stars 165 forks source link

Set start of 'ok' and improve check of empty 'file' string #4405

Closed tobolar closed 1 day ago

HansOlsson commented 1 week ago

Alternatively just don't change 'ok'.

HansOlsson commented 1 week ago

BTW: Why is fullPathName needed at all?

I thought the other commands also handled relative paths as well, and only ModelicaTest.Utilities.Internal changes directory and it should restore it - if that fails the asserts will trigger before anything is logged to the file.

So, just:

if Modelica.Utilities.Strings.isEmpty(logFile) then
  file:="";
else
  file:=logFile;
  print("... testAllFunctions(..) is logged in " + file);
  Modelica.Utilities.Files.removeFile(file);
end if;

or back to:

if logFile<>"" then
  print("... testAllFunctions(..) is logged in " + logFile);
  Modelica.Utilities.Files.removeFile(logFile);
end if;

and replace file by logFile.

tobolar commented 6 days ago

@HansOlsson Well, I didn't digger deeply in this function. The PR is only about to handle empty string of file properly i.e. using given MSL function. Regarding ok - if the default value of ok is false, then there is no need to change it. You are right. I will revert this.

HansOlsson commented 5 days ago

@HansOlsson Well, I didn't digger deeply in this function. The PR is only about to handle empty string of file properly i.e. using given MSL function.

But there are some problems:

isEmpty() returns true for both "" and " " (with space), whereas print will only print to the screen if "", so for consistency with print I would keep it as is
the test isn't for the input String (would be useful if you don't want the file output), but for the result of Modelica.Utilities.Files.fullPathName() - which is very unlikely to be an empty string.
And as noted I don't know why fullPathName is used at all

So the PR isn't that bad in itself - it just made me realize how messy the function really is.

tobolar commented 1 day ago

@HansOlsson I fully agree to all your three concerns.

So the PR isn't that bad in itself - it just made me realize how messy the function really is.

So I prefer retrieve this PR. Changing the function as you suggest is all fine but shall be done in a new PR.