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

[Core]"Merge" functionality #2241

Closed
RiccardoRossi opened this issue Jun 1, 2018 · 6 comments
Closed

[Core]"Merge" functionality #2241

RiccardoRossi opened this issue Jun 1, 2018 · 6 comments

Comments

@RiccardoRossi
Copy link
Member

As pointed out by @philbucher and @loumalouomega we have an outstanding problem with the current settings mechanism:

let's imagine that we have a "base_class"

 class base_class:
        self.default_settings = Parameters({
           "A":2,
           "B":3,
           "C":4
        }

we would like now to define a "derived_class" and to override some of the default settings or add new defaults for example so to achieve:

 class base_class:
        self.default_settings = Parameters({
           "A":200, ---> overridden
           "B":3,
           "C":4,
           "D":300 --> added
        }

currently we would need to replicate all the settings or to add/merge them by hand. This is cumbersome and error prone.

I think it would be good to have the parameters to provide such capability using a "DeriveSettings" functionality (not a good name, Merge would be also a candidate...). Long story short the idea would be to be able to write

  class DerivedClass(BaseClass):

           self.default_settings.Merge( 
                  Parameters( """
                  {   
                       "A": 200, --> this one exists in the default_settings so that we shall override it
                       "D": 300 --> this does not exist in the default_settings so that we shall add it 
                  }
                  """)
@philbucher
Copy link
Member

+1 from my side, since we discussed this in person already

@loumalouomega loumalouomega changed the title "Merge" functionality [Core]"Merge" functionality Jun 1, 2018
@pooyan-dadvand
Copy link
Member

An important feature! 👍

@pooyan-dadvand
Copy link
Member

Is this can be solved with #3889?

@loumalouomega
Copy link
Member

Is this can be solved with #3889?

No, but can be done easily (at least easier than before) using that

@roigcarlo
Copy link
Member

Closing, repoen if necessary.

@philbucher
Copy link
Member

I forgot to mention, this is kinda implemented in #4736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants