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

Move data validation to specialized functions [ANT-1213] #2149

Merged
merged 24 commits into from
Jun 17, 2024

Conversation

payetvin
Copy link
Contributor

@payetvin payetvin commented Jun 7, 2024

The goal of this PR is to separate loading and validation of data.
This separation will allow to have a modular loading.

@payetvin payetvin self-assigned this Jun 7, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 7, 2024
@@ -95,8 +95,9 @@ class PreproHydro
** \param folder The source folder (ex: `input/hydro/prepro`)
** \return A non-zero value if the operation succeeded, 0 otherwise
*/
bool loadFromFolder(Study& s, const AreaName& areaID, const char folder[]);
bool loadFromFolder(Study& s, const AreaName& areaID, const std::string& folder);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible (or usefull) to use std::filesystem::path for folder ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be done in another PR

if (col[day] < 0. || (i % 2U /*column hours*/ && col[day] > 24.))
{
logs.error() << areaName << ": invalid power or energy value";
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carefull
With the early return you are returning after the first error. Previous behaviour is to check all values and log all errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!errorPower avoided going into this scope after the first error, I simplified while keeping the same behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you don't keep the same behavior. The user will only see the 1st error, not the following ones if any.

src/libs/antares/study/parts/hydro/prepro.cpp Outdated Show resolved Hide resolved
src/libs/antares/study/parts/hydro/prepro.cpp Outdated Show resolved Hide resolved
Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split loading / validation.

bool loadParameters();
bool validateParameters(); // if (area.hydro.xxx < 1.) { return false;}, etc.

src/libs/antares/study/parts/hydro/prepro.cpp Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Jun 12, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good start @payetvin ! Please revert to the former behavior and we're good

if (col[day] < 0. || (i % 2U /*column hours*/ && col[day] > 24.))
{
logs.error() << areaName << ": invalid power or energy value";
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you don't keep the same behavior. The user will only see the 1st error, not the following ones if any.

Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing leewayDown / leewayUp comparison check.

Copy link

sonarcloud bot commented Jun 17, 2024

@flomnes flomnes dismissed JasonMarechal25’s stale review June 17, 2024 13:12

I checked, behavior is unchanged

@flomnes flomnes merged commit 08003e3 into develop Jun 17, 2024
7 checks passed
@flomnes flomnes deleted the feature/modular-validation branch June 17, 2024 13:16
@payetvin payetvin added this to the Sprint 5 milestone Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants