Allow multiple add ons for the same class or namespace

[lczech]

Currently, when specifying the add_on_binder or add_on_binder_for_namespace tokens multiple times, later ones silently replace previous ones, which is unexpected. This PR extends this to capture multiple add ons, which can be useful to break them down into smaller pieces.

[lyskov]

Thank you for PR! Could you please elaborate on how this will be used? Is the main idea is to allow user to add separate functions? If so, what will be he call order of these functions? Also, could you please elaborate on how this will be better to just supplying a single function from which user can call other functions?

[lczech]

Hey @lyskov,

Thank you for PR! Could you please elaborate on how this will be used? Is the main idea is to allow user to add separate functions?

Exactly that. I am structuring my binder config file in sections, with a small section for each header file of the code I am binding. So for header abc.hpp of my library, I'm adding a few lines for all configuration for functions and classes in that header. This keeps things neatly ordered. However, these headers might all be part of the same namespace. With this PR, I can now add extra bindings to that namespace per header.

As for classes: Not sure if this is strictly needed as well, but I figured that while at it, I might as well implement that as well. At the least, silently overwriting previous add ons is unexpected, and this avoids that. I don't currently have a use case for classes, but others might.

If so, what will be he call order of these functions?

In the order they are defined in the config. The PR just replaces the string with a vector, and adds all of them in order. Not more than that.

Also, could you please elaborate on how this will be better to just supplying a single function from which user can call other functions?

That would work as well I suppose, but would then not allow the setup above that allows for clear separation of concerns in the binder config, and instead would add a level of indirection for that function, which might be harder to trace and less flexible.

Hope that clarifies it, and sounds useful :-)
Lucas

[lyskov]

Exactly that. I am structuring my binder config file in sections, with a small section for each header file of the code I am binding. So for header abc.hpp of my library, I'm adding a few lines for all configuration for functions and classes in that header. This keeps things neatly ordered. However, these headers might all be part of the same namespace. With this PR, I can now add extra bindings to that namespace per header.

Could you please elaborate on that? Ie why defining extra-bindings-function definition somehow got tight-up to project source headers? Or do you place these extra-bindings-functions into project original headers, is that the idea?

[lczech]

Not sure that I quite understand what you are asking.

Ie why defining extra-bindings-function definition somehow got tight-up to project source headers? Or do you place these extra-bindings-functions into project original headers, is that the idea?

My extra code needed for Binder is kept separate from my original project headers, to keep things as cleanly separated as I can. But in order to bring structure into this, I am "mirroring" the directory structure of the original code, and for each header that needs extra binding code also generate a header (and if needed source file) with the same name, which then contains the functions used for the add_on_binder call.

For instance this and this line are both using add_on_binder_for_namespace for the same namespace. The functions are defined in two files here (input_source_add_ons.hpp and output_target_add_ons.hpp) in the directory where all my custom bindings code for that namespace is. So there are two functions that provide add ons for the same namespace, because those add ons are covering two different original headers. Just to keep things separate. Otherwise, I'd have one large add on file for the namespace, which I think could quickly become overwhelming. (The source files for those two functions are already several hundred lines long each.)

Does that make sense? Or is there a cleaner way to do this? Either way, I think the functionality added here is reasonable, as otherwise, add-ons on the same namespace would silently overwrite each other, which is unexpected.

[lyskov]

Aha, - now I see your point, - thank you for explaining! Yes, this makes sense and I do agree that this will be useful. I will add the review shortly.

[lyskov]

Looks good to me, thank you @lczech ! A few things before we could merge this:

-- could you please add documentation for a new config option and update docs for existent one? Please particularly highlight calling order.

-- could you please add minimal test(s) for this? I imagine something simple like single namespace and single class inside it with both having two add-on-binders.

-- run all tests and double check that there is no unexpected changes.

Thanks,

[lyskov]

@lczech bumping this PR, please see comments above. Thanks,

[lczech]

Hi @lyskov, thanks for considering the PR! Unfortunately, I won't have time in the next couple of weeks to add your requested changes, but hope to get back to it towards the end of the year.