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

Modeler 4.7: Import connections and ports #2662

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from

Conversation

payetvin
Copy link
Contributor

No description provided.

@payetvin payetvin self-assigned this Feb 24, 2025
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 25, 2025
@payetvin payetvin marked this pull request as ready for review February 28, 2025 09:20
for (const auto& port: model.ports)
{
// Can't have port with the same ID
if (std::ranges::find_if(ports, [&port](const auto& p) { return p.Id() == port.id; })
Copy link
Contributor

Choose a reason for hiding this comment

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

In line with legacy behaviour maybe we should collect all errors.
Somethings like :
for port in model.ports {
for each port {
if duplicate id => errors.add(ObjectWithThisIdAlreadyExists);
if idNotFound => errors.add(PortTypeNotFound)
}
}
if (error) {
log
throw/exit/whatever
}

for (const auto& port: model.ports)
{
// Can't have port with the same ID
if (std::ranges::find_if(ports, [&port](const auto& p) { return p.Id() == port.id; })
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of find_if you can store ids in a map.
std::map<PortID, bool> already_foundId;

for port in model.ports{
if (already_foundId[port.id])) => error
already_foundId[port.id] = true;
...

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.

Many std::ranges::find_if could be replaced with std::map::contains (if we used maps instead of vectors)

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.

3 participants