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

Clean-up catalogue extension. #2409

Conversation

thorstenhater
Copy link
Contributor

@thorstenhater thorstenhater commented Sep 18, 2024

  1. rename C++ catalogue::import to catalogue::extend as import will likely be a reserved word due to modules
  2. Use an empty default for the prefix. 99% real world examples use an empty prefix.
  3. Adjust tests and examples accordingly.

@thorstenhater thorstenhater marked this pull request as ready for review September 18, 2024 11:09
@thorstenhater thorstenhater requested a review from ErbB4 September 18, 2024 11:09
@@ -96,7 +96,7 @@ class ARB_ARBOR_API mechanism_catalogue {
}

// Copy over another catalogue's mechanism and attach a -- possibly empty -- prefix
void import(const mechanism_catalogue& other, const std::string& prefix);
void extend(const mechanism_catalogue& other, const std::string& prefix = "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice. I think I understood this perfectly hehe

@@ -114,19 +113,19 @@ struct catalogue_state {
catalogue_state() = default;

catalogue_state(const catalogue_state& other) {
import(other, "");
extend(other, "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between import and extend, since it is used everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the method to avoid clashing with an upcoming in the C++ language (modules will bring import).
So, now all uses of catalogue::import (and catalogue_state::import) must be re-written as extend instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it!

@@ -114,19 +113,19 @@ struct catalogue_state {
catalogue_state() = default;

catalogue_state(const catalogue_state& other) {
import(other, "");
extend(other, "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

got it!

@thorstenhater thorstenhater merged commit 058868a into arbor-sim:master Sep 18, 2024
23 of 24 checks passed
@thorstenhater thorstenhater deleted the qa/empty-default-prefix-for-catalogues branch September 18, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants