try singleton pattern for unpackaged #2
Conversation
| @@ -207,13 +204,19 @@ Package::Ptr Context::AddPackage(std::string name, double fill_min, double fill_ | |||
| } | |||
|
|
|||
| Package::Ptr Context::GetPackageByName(std::string name) { | |||
| if (name == Package::unpackaged_name()) { | |||
There was a problem hiding this comment.
special handling of unpackaged_name
| if (packages_.count(name) == 0) { | ||
| throw KeyError("Invalid package name " + name); | ||
| } | ||
| return packages_[name]; | ||
| } | ||
|
|
||
| Package::Ptr Context::GetPackageById(int id) { | ||
| if (id == Package::unpackaged_id()) { |
There was a problem hiding this comment.
special handling of unpackaged_id
src/package.cc
Outdated
| // otherwise return the object that already exists | ||
| Package::Ptr unpackaged() { | ||
|
|
||
| if !unpackaged_ { |
There was a problem hiding this comment.
This only ever happens once per simulation.
| Package::Package(std::string name, double fill_min, double fill_max, std::string strategy) : name_(name), id_(next_package_id_++), fill_min_(fill_min), fill_max_(fill_max), strategy_(strategy) {} | ||
|
|
||
| Package::Package(int id, std::string name) : id_(id), name_(name), fill_min_(0), fill_max_(std::numeric_limits<double>::max()) {} | ||
| Package::Package(std::string name, double fill_min, double fill_max, std::string strategy) : |
There was a problem hiding this comment.
This is my proposal to handle the different cases with a single constructor
- must always provide at least a name
- defaults for fill_min, fill_max, and strategy (see header)
- if the name is unpackaged_name_ set the id manually to the right value, otherwise use the next_id
- This works for creating the unpackaged object
- This works for creating any object without setting anything but the name - you wanted an unnamed package but someone can still do that by specifying "unnamed" in the Create() method.
There was a problem hiding this comment.
Although, if this is only called through Create() then that probably needs defaults if we want to have defaults.
src/package.cc
Outdated
| if (unpackaged_) { | ||
| throw ValueError("can't create a package with name " + unpackaged_name_); | ||
| } | ||
| id_ = unpackaged_id_; |
There was a problem hiding this comment.
I'm not sure I've covered all the bases here... it's possible for the user to generate a package with this reserved name before the unpackaged object gets built, which will lead to problems.
|
Tests are all failing, but I promised some students I'd grade their interim reports so I'll come back to this |
|
I think it's now passing tests...? |
Downstream Build Status Report - 9b95834 - 2024-04-16 02:18:29 +0000Build
|
There was a problem hiding this comment.
Thanks for this @gonuke! I will add the defaults to Create in the main PR after merging. I had only seen a pattern like this described as a way to have only one instance of an object, which obviously wasn't what we were looking for. So I appreciate your addition here!
|
Thanks @gonuke! |
Here's a proposal for a more streamlined way to get a single unpackaged type for each simulation available across all contexts.
I'll add some comments inline.
I haven't tested it all yet.