Skip to content

Suppress Redundant Configurable Validation#391

Merged
minrk merged 5 commits intoipython:masterfrom
rmorshea:configurable_super_order
May 4, 2017
Merged

Suppress Redundant Configurable Validation#391
minrk merged 5 commits intoipython:masterfrom
rmorshea:configurable_super_order

Conversation

@rmorshea
Copy link
Contributor

@rmorshea rmorshea commented Apr 7, 2017

Closes #390

Summary

Configurables currently make a redundant series of setattr calls on themselves to override config values with those that have been explicitly hard-coded in the constructor. In the past this was required due to the timing of descriptor initialization via instance_init, which occurred in HasTraits.__init__. Now though, descriptor initialization occurs in HasTraits.__new__ via setup_instance. Thus we can remove the redundant setattr in Configurable.__init__ and simply use super().__init__(**kwargs) instead.

Todo

  1. I'm encountering a test failure complaining that a warning for a mistyped section did not occur. However, pytest shows that stderr received a call with the exact warning required by the test. This is specific to Python 3 since it requires TestCase.assertLogs.
  2. Add a test case.

cc: @jasongrout

@minrk
Copy link
Member

minrk commented Apr 7, 2017

While you're at it, can you make sure there's a test case for what this is supposed to resolve?

class C(Configurable):
    x = Unicode(config=True)

cfg = Config()
cfg.C.x = 'from config'

c = C(x='from init')
assert c.x == 'from init'

@rmorshea
Copy link
Contributor Author

rmorshea commented Apr 7, 2017

@minrk sure. Any thoughts on the test failure I'm getting here? I don't know what's causing it.

@minrk minrk force-pushed the configurable_super_order branch 2 times, most recently from 12d5535 to 2137d14 Compare April 11, 2017 13:49
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

The test is verifying that constructor keyword-args (in this case 'log') are loaded prior to config. This PR explicitly removes that behavior, which isn't quit right.

We have these requirements:

  1. traits passed to the constructor load before config
  2. traits passed to the constructor have higher priority than config

This is handled somewhat clumsily in master by loading all traits passed as kwargs twice - before and after config (hence #390). I've updated the PR to only re-initialize those kwargs that are actually overridden by config, which should make it a smaller change and still fix #390.

msg += u" Did you mean `{matches}`?".format(matches=matches[0])
elif len(matches) >= 1:
msg +=" Did you mean one of: `{matches}`?".format(matches=', '.join(sorted(matches)))
print(msg)
Copy link
Member

Choose a reason for hiding this comment

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

TODO: remove print before merge

minrk added 2 commits April 11, 2017 16:18
…y config

avoids *unnecessary* double-validation, but will be double-validated if loaded more than once.
@minrk minrk force-pushed the configurable_super_order branch from 1de4fa8 to 11dc842 Compare April 11, 2017 14:19
@minrk minrk added this to the 5.0 milestone Apr 11, 2017
@minrk minrk merged commit c64cf35 into ipython:master May 4, 2017
@rmorshea
Copy link
Contributor Author

rmorshea commented May 4, 2017

@minrk creative fix! 👍

@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-minor rereviewed, minor change need to be put in changelog. 5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc) and removed 5.0-minor rereviewed, minor change need to be put in changelog. 5.0-re-review Need to re-review for potential API impact changes. labels Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configurable validates too often

3 participants