Skip to content

allow_none=False for all trait types but Instance and Type #7708

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.

Sign up for GitHub

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

Jump to bottom
Merged
minrk merged 5 commits into from
Mar 20, 2015
Merged

allow_none=False for all trait types but Instance and Type #7708

minrk merged 5 commits into from
Mar 20, 2015

Conversation

@SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Feb 7, 2015

Closes #7670 .

@SylvainCorlay SylvainCorlay force-pushed the allow_none branch 4 times, most recently from 09475ef to e068044 Compare February 7, 2015 20:39
@SylvainCorlay SylvainCorlay changed the title allow_none=False for all trait types but Instance allow_none=False for all trait types but Instance and Type Feb 7, 2015
@SylvainCorlay SylvainCorlay force-pushed the allow_none branch 6 times, most recently from 44b4492 to f2e014d Compare February 7, 2015 22:26
@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Feb 9, 2015

@minrk
Regarding the places where I had to add allow_none=True. it is interesting to check whether it was expected, specifically regarding the enums in

shellapp.py
interactiveshell.py
test_message_spec.py

@minrk
Copy link
Member

minrk commented Feb 9, 2015

Good point. I bet it's not all of them.

Thanks for this!

minrk
minrk reviewed Feb 9, 2015
View reviewed changes
IPython/kernel/tests/test_message_spec.py Outdated
Copy link
Member

@minrk minrk Feb 9, 2015

Choose a reason for hiding this comment

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

I think all of these here should all have allow_none=False. I'm guessing that means a default_value must be specified explicitly.

Copy link
Member Author

@SylvainCorlay SylvainCorlay Feb 9, 2015

Choose a reason for hiding this comment

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

Yeah, I was just allowing None wherever my change was causing issues. I will add default values.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Feb 10, 2015

Done, I have replaced the allow_none=True with default values. The chosen default value is the first value proposed in the Enum. Travis is happy.

@SylvainCorlay SylvainCorlay mentioned this pull request Feb 10, 2015
Merged
@Carreau Carreau added this to the 4.0 milestone Feb 12, 2015
@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Feb 14, 2015

When #7770 is merged, it will become sensible to have allow_none=False for Instance and Type as well, since unused default values will not be instantiated.

Then, it would be nice to not have any allow_none=True by default in all trait types, but I will make it a different PR because there are much more cases in IPython where it will be required to forcibly set allow_none=True for Type or Instance trait type.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Feb 21, 2015

Removed some redundant allow_none=False now that it is the default.

minrk added a commit that referenced this pull request Mar 20, 2015
@minrk
Merge pull request #7708 from SylvainCorlay/allow_none
454aa2c 
allow_none=False for all trait types but Instance and Type
@minrk minrk merged commit 454aa2c into ipython:master Mar 20, 2015
@SylvainCorlay SylvainCorlay deleted the allow_none branch March 20, 2015 15:38
@takluyver
Copy link
Member

takluyver commented Mar 20, 2015

I think this might be causing some failures in IPython.parallel: "The 'stop_data' trait of a LocalEngineSetLauncher instance must be a dict, but a value of class 'NoneType' (i.e. None) was specified."

@minrk
Copy link
Member

minrk commented Mar 20, 2015

Thanks, I'll make sure the parallel tests pass and open a PR with the few remaining changes. This was one part, but there's another as well.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Mar 20, 2015

Sorry I missed this one. On this regard, after the default value validation PR is merged, I would like to do the same with Type and Instance (have allow_none=False by default).
I will make sure to run the parallel tests.

@minrk minrk mentioned this pull request Mar 20, 2015
Merged
@SylvainCorlay SylvainCorlay mentioned this pull request Jun 3, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.0

Development

Successfully merging this pull request may close these issues.

Traitlets: allow_none default value for trait types

4 participants

@SylvainCorlay @minrk @takluyver @Carreau