Skip to content

Conversation

@wvpm
Copy link
Contributor

@wvpm wvpm commented Jan 9, 2024

No description provided.

@wvpm wvpm requested a review from Hop311 January 9, 2024 22:14
@wvpm wvpm force-pushed the improve_production_type_parsing branch 5 times, most recently from bfcb453 to 56af80c Compare January 10, 2024 10:19
Base automatically changed from improve_production_type_parsing to master January 10, 2024 10:49
@Spartan322 Spartan322 added the enhancement New feature or request label Jan 11, 2024
@wvpm wvpm force-pushed the producer_types branch 4 times, most recently from 43e6cd2 to 2b75c36 Compare January 16, 2024 13:34
@wvpm wvpm marked this pull request as ready for review January 16, 2024 13:37
@wvpm wvpm force-pushed the producer_types branch 2 times, most recently from 4a5ed35 to 0e92483 Compare January 18, 2024 21:33
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

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

Looks like we've got all the data we need here, now to make a corresponding Producer.cpp file containing constructor implementations and with the implementations of get_profitability_yesterday and get_average_profitability_last_seven_days moved into it.

One thing to bear in mind: derived class constructors can't directly initialise their base class' variables, they have to call the base class' constructor.

Also can you rebase this onto master to include recent changes.

@wvpm wvpm force-pushed the producer_types branch 3 times, most recently from c53e9b8 to 773b1f4 Compare January 27, 2024 22:02
Copy link
Member

@Spartan322 Spartan322 left a comment

Choose a reason for hiding this comment

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

I would also really prefer all the maros be prefixed with OV_ or _OV_ depending on expected use case, those which you use directly should be OV_, those you usually don't should be _OV_, and something like value_or_move should be in a namespace even if its in macros like

namespace OpenVic::detail {
    template<typename T>
    constexpr T value_or_move(T&& value);
}

#define MACRO(Arg) ::OpenVic::detail::value_or_move(Arg);

@wvpm wvpm force-pushed the producer_types branch 3 times, most recently from 7640ad0 to e98cd1f Compare January 28, 2024 16:17
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

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

Great work! Once this is merged, "draft for factory production" #144 will automatically re-target itself onto master, however you'll still need to rebase it to fix merge conflicts.

@wvpm wvpm merged commit 5216e89 into master Feb 6, 2024
@wvpm wvpm deleted the producer_types branch February 6, 2024 16:44
Hop311 pushed a commit to Hop311/OpenVic-Simulation that referenced this pull request Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants