CM configurable Take 2 #2979

Merged
merged 1 commit into from Apr 24, 2013

Conversation

Projects
None yet
3 participants
@Carreau
Owner

Carreau commented Feb 25, 2013

Pinging @fperez,
This should fix your issues with markdown cell coloration.

It is a little more complicated than I expected if we want to avoid code duplication in a few places, so I'll work on that.

Change the way configurability works.
Config dict should be passed down to the parent class where it will be
merged with the default value and propagate to this only in the base
class.

This allow to both alter the configuration on a per instance basis, or
globaly by tempering with the class instance.

This also get rid of IPython global in some cases.

Still not perfect but I think this is the limit of my js knowledge, there is a minimal amount of code of 4 line to propagate the configuration :

var options = {foo:bar}; // default options can be class parameter
var overwrite_options ={boo:baz}; // came from args, or not
options = this.mergeopt(CodeCell, options, overwrite options);
IPython.Cell.apply(this,[options]);

I'm just not found of passing [option] instead of just options and the squash the options dict together in a 3 step way...

@minrk

This comment has been minimized.

Show comment Hide comment
@minrk

minrk Feb 25, 2013

Owner

What is the relationship of this to #2973

Owner

minrk commented Feb 25, 2013

What is the relationship of this to #2973

@Carreau

This comment has been minimized.

Show comment Hide comment
@Carreau

Carreau Feb 25, 2013

Owner

Hum... I'll close #2973, it will become part of this one once done.

#2973 was to close #2972, but there are deeper issues.

Owner

Carreau commented Feb 25, 2013

Hum... I'll close #2973, it will become part of this one once done.

#2973 was to close #2972, but there are deeper issues.

@minrk

This comment has been minimized.

Show comment Hide comment
@minrk

minrk Feb 25, 2013

Owner

Okay, thanks for clarifying.

Owner

minrk commented Feb 25, 2013

Okay, thanks for clarifying.

@Carreau

This comment has been minimized.

Show comment Hide comment
@Carreau

Carreau Feb 25, 2013

Owner

No problem, thanks for filling the gap of my (too short) memory.

Owner

Carreau commented Feb 25, 2013

No problem, thanks for filling the gap of my (too short) memory.

@ghost ghost assigned Carreau Mar 27, 2013

@ellisonbg

This comment has been minimized.

Show comment Hide comment
@ellisonbg

ellisonbg Apr 8, 2013

Owner

Should we review and merge this one as is? Do you want to do the CM 3 stuff separately?

Owner

ellisonbg commented Apr 8, 2013

Should we review and merge this one as is? Do you want to do the CM 3 stuff separately?

@Carreau

This comment has been minimized.

Show comment Hide comment
@Carreau

Carreau Apr 9, 2013

Owner

Should we review and merge this one as is? Do you want to do the CM 3 stuff separately?

Haven't had time to look at it in a while. It shoudln't conflict with CM3 though. I'ts just a question of passing config around to be able to overwrite it. So Yes, this and CM3 are different things, I just have the same concern of me not beeing availlable 17-30.

Owner

Carreau commented Apr 9, 2013

Should we review and merge this one as is? Do you want to do the CM 3 stuff separately?

Haven't had time to look at it in a while. It shoudln't conflict with CM3 though. I'ts just a question of passing config around to be able to overwrite it. So Yes, this and CM3 are different things, I just have the same concern of me not beeing availlable 17-30.

JS Configurablity Take 2
Change the way configurability works.
Config dict should be passed down to the parent class where it will be
merged with the default value and propagate to this only in the base
class.

This allow to both alter the configuration on a per instance basis, or
globaly by tempering with the class instance.

This also get rid of IPython global in some cases.
@Carreau

This comment has been minimized.

Show comment Hide comment
@Carreau

Carreau Apr 9, 2013

Owner

rebased, cleaned updated.
It shouldn't change the current behavior of the notebook at all.

Owner

Carreau commented Apr 9, 2013

rebased, cleaned updated.
It shouldn't change the current behavior of the notebook at all.

minrk added a commit that referenced this pull request Apr 24, 2013

Merge pull request #2979 from Carreau/cm-configurable
CM configurable Take 2

Change the way configurability works.
Config dict should be passed down to the parent class where it will be
merged with the default value and propagate to this only in the base
class.

This allow to both alter the configuration on a per instance basis, or
globaly by tempering with the class instance.

This also get rid of IPython global in some cases.

-- 

Still not **perfect** but I think this is the limit of my js knowledge, there is a minimal amount of code of 4 line to propagate the configuration :

```
var options = {foo:bar}; // default options can be class parameter
var overwrite_options ={boo:baz}; // came from args, or not
options = this.mergeopt(CodeCell, options, overwrite options);
IPython.Cell.apply(this,[options]);
```

@minrk minrk merged commit 45c0033 into ipython:master Apr 24, 2013

1 check passed

default The Travis build passed
Details

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #2979 from Carreau/cm-configurable
CM configurable Take 2

Change the way configurability works.
Config dict should be passed down to the parent class where it will be
merged with the default value and propagate to this only in the base
class.

This allow to both alter the configuration on a per instance basis, or
globaly by tempering with the class instance.

This also get rid of IPython global in some cases.

-- 

Still not **perfect** but I think this is the limit of my js knowledge, there is a minimal amount of code of 4 line to propagate the configuration :

```
var options = {foo:bar}; // default options can be class parameter
var overwrite_options ={boo:baz}; // came from args, or not
options = this.mergeopt(CodeCell, options, overwrite options);
IPython.Cell.apply(this,[options]);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment