Small refactoring CR23 - File deletion #1732

nikolaredstork · 2023-10-31T12:59:21Z

Excluding deletion of deprecated files from load functions.
Comment from the main PR #1470 (comment)

…unction

nikolaredstork · 2023-10-31T13:10:26Z

src/ui/simulator/application/study.cpp

@@ -393,7 +393,14 @@ class JobSaveStudy final : public Toolbox::Jobs::Job
        }     // save as



@guilpier-code This is obviously code duplication. Do you have proposal where potential function should be defined?

@guilpier-code This is obviously code duplication. Do you have proposal where potential function should be defined?

As you know, we don't want to enhance the UI code, we think it's not worth it.
Besides, it's not exactly code we touched for this CR 23.
But you're right, there is a code duplication.

guilpier-code · 2023-10-31T15:32:55Z

src/libs/antares/study/study.h

@@ -449,6 +449,12 @@ class Study: public Yuni::NonCopyable<Study>, public IObject, public LayerData
    void removeTimeseriesIfTSGeneratorEnabled();
    //@}

+    /*!


Class Study is large enough, we plan to split it in many pieces, and we don't want it to grow.

For now, we should make this function a free function. We could add it as a couple of source files file-removal.h/.cpp

What it actually does is removing deprecated hydro max power input files, right ? Don't hesitate renaming it removeDeprecatedHydroMaxPowerTSfiles.

this function should take 3 arguments :

a list of areas (AreaList&),

a input folder path (as an std::string or in a standard format, we don't want dependencies on Yuni strings, we want to get rid of it)

a current study version (as a unsigned int)

It could be something like :

void removeDeprecatedHydroMaxPowerTSfiles(AreaList& areas, std::string inputFolder, unsigned int currentStudyVersion) { if (currentStudyVersion >= 870) return; vector<std::string> areasNames; areas.each( [](const Area& area) { areasNames.push_back(area.id.to<std::string>(); }); for (const name : areasNames) { std::string filePath = folderInput + "/hydro/common/capacity/" + name + ".txt" removeFile(filePath); // To be coded in the same source file, in the .cpp file. } }

On the client side, it would be :

removeDeprecatedHydroMaxPowerTSfiles(study.areas, study.folderInput.to<std::string>, study.header.version),

Note : about removeFile(filePath), it probably already exists in the unit tests code area

The idea is to first read currentStudyVerstion and save it value to temporary variable, after that we are calling study->saveToFolder. Why we need temporary variable? We need it because, if we are upgrading a study, variable study->.header.version will be changed (after study->saveToFolder function call) to a current version of our software, and if we are passing value of the study->.header.version variable to the deleteDeprecatedHydroMaxPowerTSFiles function we will be passing already updated value of our study version (currently 870). If we are upgrading for example from 8.2 version than we need to pass 820 value to the deleteDeprecatedHydroMaxPowerTSFiles function, and we are doing that via temporary variable.
Condition if (currentStudyVersion >= 870) must be changed to if(previousVersion < 870) . Why? Because currentStudyVersion after saving a study will have 870 value, and deletion won't be performed.

Also, we need to delete files just if study->saveToFolder() function call return true, than we are sure that our new files are saved and all data from deprecated files are stored in our new files.

ok. Your last comments regard more the updater/main.cpp than the src/libs/antares/study/study.h.
So I answer you in updater/main.cpp (below)

guilpier-code · 2023-10-31T16:04:17Z

src/tools/updater/main.cpp

@@ -93,7 +93,12 @@ class MyStudyFinder final : public Data::StudyFinder
                    study->parameters.readonly = true;

                logs.info() << "Saving...";
-                study->saveToFolder(folder);
+
+                uint tempVersionVar = (uint)study->header.version;


tempVersionVar : we should be more clear. This variable is the current study version, isn't it ?
So let's call it currentStudyVersion

An answer to the previous comment from @nikolaredstork was given here.

Here is what think, eventually about tempVersionVar and this added piece of code.
I think we should have :

... bool isSaved = study->saveToFolder(folder); if (isSaved) deleteDeprecatedHydroMaxPowerTSFiles(study->areas, study->folderInput.to<std::string>, study->header.version);

Note we don't need the source (what you called tmp) study version here.
Why ?
When get here, a study was loaded, and it may have input deprecated files.
Then we upgrade it, meaning that we erase input data with fresh updated one.
Deprecated files (hydro max power TS in this case) can still remain.
Calling deleteDeprecatedHydroMaxPowerTSFiles will delete them if they exists and if the new study version requires it.
Note that the small removeFile function suggested in this comment removes a file only if it exists of course.

It's more simple like this, don't you think ?

guilpier-code · 2023-10-31T16:36:59Z

src/tools/updater/main.cpp

@@ -93,7 +93,12 @@ class MyStudyFinder final : public Data::StudyFinder
                    study->parameters.readonly = true;

                logs.info() << "Saving...";
-                study->saveToFolder(folder);
+


Normally, we should add this to the src/libs/antares/study/cleaner/cleaner.cpp, which is supposed to clean any study from unsuitable files.

You are right. I've looked up cleaner.cpp file in details, and apparently after upgrading and saving our new files, we will delete deprecated files with clean command, so I don't think that we need automatic deletion after upgrade. With that been said, this PR shouldn't be merged and code for deletion of files should be excluded.

guilpier-code · 2023-10-31T16:58:53Z

src/ui/simulator/application/study.cpp

@@ -393,7 +393,14 @@ class JobSaveStudy final : public Toolbox::Jobs::Job
        }     // save as



@guilpier-code This is obviously code duplication. Do you have proposal where potential function should be defined?

As you know, we don't want to enhance the UI code, we think it's not worth it.
Besides, it's not exactly code we touched for this CR 23.
But you're right, there is a code duplication.

guilpier-code · 2023-11-02T09:53:24Z

src/libs/antares/study/study.h

@@ -449,6 +449,12 @@ class Study: public Yuni::NonCopyable<Study>, public IObject, public LayerData
    void removeTimeseriesIfTSGeneratorEnabled();
    //@}

+    /*!


ok. Your last comments regard more the updater/main.cpp than the src/libs/antares/study/study.h.
So I answer you in updater/main.cpp (below)

guilpier-code · 2023-11-02T09:55:42Z

src/tools/updater/main.cpp

@@ -93,7 +93,12 @@ class MyStudyFinder final : public Data::StudyFinder
                    study->parameters.readonly = true;

                logs.info() << "Saving...";
-                study->saveToFolder(folder);
+
+                uint tempVersionVar = (uint)study->header.version;


An answer to the previous comment from @nikolaredstork was given here.

guilpier-code · 2023-11-02T09:59:53Z

src/tools/updater/main.cpp

@@ -93,7 +93,12 @@ class MyStudyFinder final : public Data::StudyFinder
                    study->parameters.readonly = true;

                logs.info() << "Saving...";
-                study->saveToFolder(folder);
+
+                uint tempVersionVar = (uint)study->header.version;


Here is what think, eventually about tempVersionVar and this added piece of code.
I think we should have :

... bool isSaved = study->saveToFolder(folder); if (isSaved) deleteDeprecatedHydroMaxPowerTSFiles(study->areas, study->folderInput.to<std::string>, study->header.version);

Note we don't need the source (what you called tmp) study version here.
Why ?
When get here, a study was loaded, and it may have input deprecated files.
Then we upgrade it, meaning that we erase input data with fresh updated one.
Deprecated files (hydro max power TS in this case) can still remain.
Calling deleteDeprecatedHydroMaxPowerTSFiles will delete them if they exists and if the new study version requires it.
Note that the small removeFile function suggested in this comment removes a file only if it exists of course.

It's more simple like this, don't you think ?

sonarcloud · 2023-11-02T11:51:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

CR 23 Refactoring : Removing deletion of deprecated files from Load f…

1984b81

…unction

nikolaredstork requested review from guilpier-code and Milos-RTEi October 31, 2023 12:59

pull-request-size bot added the size/M label Oct 31, 2023

nikolaredstork commented Oct 31, 2023

View reviewed changes

nikolaredstork added 2 commits October 31, 2023 16:16

Excluding unnecessary bool variable

ca51b02

Build error

76d242e

guilpier-code requested changes Oct 31, 2023

View reviewed changes

guilpier-code requested changes Nov 2, 2023

View reviewed changes

pull-request-size bot added size/XS and removed size/M labels Nov 2, 2023

Excluding automatic file removal

ea6c725

guilpier-code requested changes Nov 2, 2023

View reviewed changes

nikolaredstork requested a review from guilpier-code November 2, 2023 11:53

guilpier-code approved these changes Nov 2, 2023

View reviewed changes

guilpier-code merged commit 3da3b13 into feature/max-gen-pump-cr23-develop-merge Nov 2, 2023
7 checks passed

guilpier-code deleted the fix/cr-23-deprecated-file-deletion branch November 2, 2023 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small refactoring CR23 - File deletion #1732

Small refactoring CR23 - File deletion #1732

nikolaredstork commented Oct 31, 2023 •

edited

Loading

nikolaredstork Oct 31, 2023 •

edited

Loading

guilpier-code Oct 31, 2023

guilpier-code Oct 31, 2023 •

edited

Loading

nikolaredstork Oct 31, 2023 •

edited

Loading

nikolaredstork Oct 31, 2023 •

edited

Loading

guilpier-code Nov 2, 2023

guilpier-code Oct 31, 2023

guilpier-code Nov 2, 2023

guilpier-code Nov 2, 2023

guilpier-code Oct 31, 2023

nikolaredstork Nov 2, 2023 •

edited

Loading

guilpier-code Oct 31, 2023

guilpier-code Nov 2, 2023

guilpier-code Nov 2, 2023

guilpier-code Nov 2, 2023

sonarcloud bot commented Nov 2, 2023

		@@ -393,7 +393,14 @@ class JobSaveStudy final : public Toolbox::Jobs::Job
		} // save as

Small refactoring CR23 - File deletion #1732

Small refactoring CR23 - File deletion #1732

Conversation

nikolaredstork commented Oct 31, 2023 • edited Loading

nikolaredstork Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilpier-code Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

nikolaredstork Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

nikolaredstork Oct 31, 2023 • edited Loading

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

nikolaredstork Nov 2, 2023 • edited Loading

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 Nov 2, 2023

nikolaredstork commented Oct 31, 2023 •

edited

Loading

nikolaredstork Oct 31, 2023 •

edited

Loading

guilpier-code Oct 31, 2023 •

edited

Loading

nikolaredstork Oct 31, 2023 •

edited

Loading

nikolaredstork Oct 31, 2023 •

edited

Loading

nikolaredstork Nov 2, 2023 •

edited

Loading