For short-term storage cumulative constraints [ANT-1855]

[a-zakir]

add week-dependent rhs
New syntax to declare multiple contraints that shares same rhs vector and type

[guilpier-code]

We don't need to define a struct ValidateResult : it can be replaced with a std::pair<bool, std::string>.

That said, instead of returning something, we could throw exception (std::runtime_error ?) inside validate().
This would simplify the function a bit :

• it would return void
• it would avoid the strange default return {true, ""}

Calling it would be a bit simpler and conventional :

try
{
    additional_constraints.validate();
}
catch (const std::runtime_error& ex)
{
    logs.error() << "Invalid constraint in section: " << section->name;
    logs.error() << ex.what();
    return false;
}

[payetvin]

pair done

[a-zakir]

i don't thinh that std::pair<bool, std::string> is more meaningful than struct other { bool ok; string msg;} come on!
pair<bool, string> a; a.first = true;

other o; o.ok = true;

[flomnes]

I have to agree with @a-zakir on this one

[guilpier-code]

The suggestion of using a std::pair<bool, std::string> instead of a struct was a part of this remark.
What about the other part (with the code sample) ?

[a-zakir]

we don't necessarily want to throw exception, the return boolean is used by the caller
bool AreaList::loadFromFolder --> bool STStorageInput::loadAdditionalConstraints --> AdditionalConstraints::ValidateResult AdditionalConstraints::validate()

[a-zakir]

in your suggestion:

try
{
    additional_constraints.validate();
}
catch (const std::runtime_error& ex)
{
    logs.error() << "Invalid constraint in section: " << section->name;
    logs.error() << ex.what();
    return false;
}

i am doubtful with throwing an exception to catch it the line after.
can you give an example of code in the project that use this technique?

[guilpier-code]

Sorry, but I find this code painful to understand.
I find it difficult to make the correspondance between the code and the content of the specifications, whereas it should be quite immediate.

I'll try to explain why :

• Function cumulationConstraintFromVariable : this function creates a constraint. Indeed, it returns a child of class CumulationConstraint. So I would rename it into cumulationConstraintFactory(...)

• constraintHelper : why do we give this variable that name ? This local variable is the result of a function that returns a CumulationConstraint. So let's name this local variable cumulationConstraint. I personally never append or prefix a name (variable, class) with helper or manager, because it's not meaningful enough, it's too vague.

• class CumulationConstraint : there is design problem
  This class is an interface and requires its children to define the build(...) function.
  build(...) takes 3 arguments. The 3rd argument is used only by one child, and useless to the other 2.
  To fix this, we could pass the builder and the storage to the constraint factory (function cumulationConstraintFromVariable), and pass the storage internally to the constructor of the cumulation constraint that needs it.
  Doing this, we simplify the call form :
  constraintHelper->build(builder, index, storage);
  to :
  constraintHelper->build(index);

  We could even go further and replace :

  builder.updateHourWithinWeek(hour - 1);
  constraintHelper->build(builder, index, storage);

  with only :

  constraintHelper->build(index, hour); // Notice the new 'hour' as arg

• namer : useless to build a constraint itself, but used to build the constraint name to print in a MPS file. So we mix the construction of the constraint itself and its MPS name in the same function. We could do both things separately (in different functions), so that each piece is simpler and more understandable, but it's already done in one piece in many places. So is it worth it to separate things here and not elsewhere ?

• index : when we use it, we don't know which objet this index is related to, so we have to navigate to its definition to realize it's a storage index. Let's name it storageIndex.

• About ConvertSense(...) :

  • this conversion should have already been made at that (for example when reading input data or right after), it's not the place to do it. Doing it some place else would lighten the code a little bit more.
  • renaming : we talk about operator type on one hand, and about sense on the other hand. We have to choose which unique term to use. I ask to @JMJ-rte, he recommends operator.
    For more clarity, this function should be called convertOperatorToChar

[a-zakir]

ok for the first 3 points, about namer, ths object is instantiated once outside the for loops and one of its method is called inside those loops, i don't see what i can do about it unless to recreate it for every ite of the inner loop via a function.

on concertSense grammar, you'll notice that less, greater and equal are not striclty operator, they are words and their global defintion is Sense

[guilpier-code]

Acceptable like this for me

[a-zakir]

ok

[guilpier-code]

we should not hesitate to make test title look like a sentence :
Validate_ClusterIdEmpty ==> cluster_id_is_empty___we_get_validation_error

[guilpier-code]

For many tests, we :

• create an additional constraint
• give the same value to its fields

We should use a fixture creating an additional constraint and giving a default value to its fields

[sonarqubecloud]

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
52.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[guilpier-code]

// std::array<double, HOURS_PER_YEAR> rhs = {};

comment to be removed

[guilpier-code]

// TODO a lot unused entries

What do you mean ?
If the comment is meant to remain there, it should be cristal clear or we will forget what it refers to

[a-zakir]

rhs values corresponding to the hours set are used. (rhs is a vector 8760 lines)