Add a DeprecationWarning for when edit is false in configure_traits() #1272

avats-dev · 2020-08-13T06:21:28Z

fixes #1271

somewhat similar to #792

Checklist

Tests
Update API reference (docs/source/traits_api_reference)
Update User manual (docs/source/traits_user_manual)
Update type annotation hints in traits-stubs

kitchoi · 2020-08-13T10:46:59Z

Hi @avats-dev

Thank you for this! I think the CI is unhappy about building the documentation, i.e. this line fails:

traits/.travis.yml

Line 54 in 91cd08c

- pushd docs && python3 -m sphinx -b html -W source build && popd

The error message is the following:

Extension error:
Handler <function _process_docstring at 0x7f662dadf730> for event 'autodoc-process-docstring' threw an exception (exception: <method 'trait_items_event' of 'traits.ctraits.CHasTraits' objects> is not a module, class, method, function, traceback, frame, or code object)
The command "pushd docs && python3 -m sphinx -b html -W source build && popd" exited with 2.

The last successful build was using Sphinx 3.1.2. The CI build for this PR is using Sphinx 3.2.0.
Since the error message is complaining about a method that is not modified in this PR, I suspect the error to be orthogonal and may just be caused by the Sphinx upgrade (combined with an actual documentation issue).
If that is true, I think we can deal with this in a separate PR, not this one.

Back to the deprecation on edit. :)

I think the intended outcome of this deprecation is that no one should be calling configure_traits with the edit argument anymore.

e.g.
There should be no deprecation warnings for this:

model.configure_traits()

But there should be deprecation warning for this:

model.configure_traits(edit=True)

And deprecation warning for this:

model.configure_traits(edit=False)

I think in this PR, we get deprecation warning for this:

model.configure_traits(edit=False)

but not for this:

model.configure_traits(edit=True)

I think we want to emit deprecation even when edit is explicitly set to True. However, while we do that, we need to make sure no deprecation warnings is emitted if the argument is not set (because that's the goal).

So we are almost there, but not quite yet. Could you modify this PR please?

kitchoi · 2020-08-13T11:05:48Z

Since the error message is complaining about a method that is not modified in this PR, I suspect the error to be orthogonal and may just be caused by the Sphinx upgrade (combined with an actual documentation issue).
If that is true, I think we can deal with this in a separate PR, not this one.

Confirmed this is an issue for master too. I opened #1273.

avats-dev · 2020-08-13T15:17:28Z

Thanks @kitchoi for the analysis and review. I think you're right about the deprecation warning if edit is set explicitly, I never thought about this. I will update the PR soon with the changes. :) 👨‍💻

And good catch on that failing CI.

avats-dev · 2020-08-18T02:33:07Z

Hey @kitchoi , how can I check if the default argument is passed explicitly or not?

I got some insight from here and I'm thinking of using inspect module. I could use some help.

kitchoi · 2020-08-18T08:38:45Z

I'd probably change the default value of edit to None, then check for it inside the function body. It should be similar to the if filename is not None in the top of the function, but for edit, its value needs to be flipped back to True when the value is None.

kitchoi · 2020-08-18T08:50:34Z

I'd probably change the default value of edit to None, then check for it inside the function body. It should be similar to the if filename is not None in the top of the function, but for edit, its value needs to be flipped back to True when the value is None.

That said, this won't catch the situation where someone calls configure_traits(edit=None), but that should be rare (or nonexisting), and could almost be considered a misuse given the description of the method said the argument should be a bool.

I got some insight from here and I'm thinking of using inspect module. I could use some help.

Looks like inspect could be used to solve a more general problem, while it could probably solve the problem even for the configure_traits(edit=None) edge case, I am leaning towards thinking that might be an overkill.

avats-dev · 2020-08-18T18:55:07Z

Looks like inspect could be used to solve a more general problem, while it could probably solve the problem even for the configure_traits(edit=None) edge case, I am leaning towards thinking that might be an overkill.

Ok, I also thought that it might be an overkill here, and so I think I'll just go with the edit=None idea and then flip it in the function to True if it is None. Thanks for the insight, will update the PR soon.

mdickinson · 2020-09-01T09:13:40Z

Hi @avats-dev! Do you think you'll have time to finish this one off? Don't worry if not - just let us know, and we'll pick it up from here.

avats-dev · 2020-09-01T17:50:29Z

@mdickinson I got busy with something else so I couldn't complete this. I'll finish this soon. 🚀

avats-dev · 2020-09-06T01:23:56Z

Updated 🎉
Suggestions @mdickinson @kitchoi

mdickinson · 2020-09-07T07:13:13Z

The Appveyor failure is a known issue that's independent of this PR, and should be fixed by #1290.

traits/has_traits.py

mdickinson · 2020-09-07T07:36:27Z

traits/has_traits.py

@@ -2034,7 +2037,7 @@ def configure_traits(
                with open(filename, "rb") as fd:
                    self.copy_traits(pickle.Unpickler(fd).load())

-        if edit:
+        if edit is None:


I think we can refactor this a bit to remove duplication: the goal would be to not have the same logic expressed in different branches. (Read up on the "DRY Principle" for some of the rationale for avoiding duplication.)

If you do something like:

if edit is None: edit = True else: warnings.warn(...)

here, then you should be able to leave the rest of the code unchanged.

But as we're showing warning for both edit=True and edit=False, then refactoring like

if edit is None: edit = True else: warnings.warn(...)

would give warning even on None, but we don't want that.

I've added spaces as mentioned and refactored code. See if it's good?

Sorry for being late. I was out of station for some days so I was not able to work on this.

would give warning even on None

I don't see how. If edit is None, then the first branch of the if statement is taken, and no warning is given. If edit is not None, then the else branch is taken, and a warning is given. I think that exactly matches what we want, doesn't it?

mdickinson

Thanks for the new commits! There's a fix needed for the deprecation notes in the documentation, and a suggested reworking to remove some of the duplication.

traits/has_traits.py

mdickinson · 2020-09-18T07:22:03Z

The logic still isn't right here; luckily, the tests are failing, which is exactly what they're designed to do (yay!).

Can you dig into the test failures and figure out what's wrong? I also highly recommend running the tests yourself locally while you're developing, in addition to the manual testing.

You can run just the relevant tests with:

$ python -m unittest -v traits.tests.test_configure_traits

Here's what I currently see on my machine when I run those tests:

(traits) mdickinson@mirzakhani traits % python -m unittest traits.tests.test_configure_traits
..F.F.F/Users/mdickinson/Enthought/ETS/traits/traits/has_traits.py:2067: DeprecationWarning: The edit argument to configure_traits is deprecated, and will be removed in Traits 7.0.0
  warnings.warn(message, DeprecationWarning)
F
======================================================================
FAIL: test_filename_but_no_file (traits.tests.test_configure_traits.TestConfigureTraits)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/traits/traits/tests/test_configure_traits.py", line 54, in test_filename_but_no_file
    self.assertTrue(os.path.exists(filename))
AssertionError: False is not true

======================================================================
FAIL: test_filename_with_existing_file_stores_updated_model (traits.tests.test_configure_traits.TestConfigureTraits)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/traits/traits/tests/test_configure_traits.py", line 122, in test_filename_with_existing_file_stores_updated_model
    self.assertEqual(model.count, 23)
AssertionError: 52 != 23

======================================================================
FAIL: test_pickle_protocol (traits.tests.test_configure_traits.TestConfigureTraits)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/traits/traits/tests/test_configure_traits.py", line 73, in test_pickle_protocol
    self.assertTrue(os.path.exists(filename))
AssertionError: False is not true

======================================================================
FAIL: test_simple_call (traits.tests.test_configure_traits.TestConfigureTraits)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/traits/traits/tests/test_configure_traits.py", line 43, in test_simple_call
    self.assertEqual(mock_view.call_count, 1)
AssertionError: 0 != 1

----------------------------------------------------------------------
Ran 8 tests in 0.015s

FAILED (failures=4)

What we want to see here is no test failures, and no unsuppressed warnings.

mdickinson · 2020-09-18T08:34:16Z

@avats-dev I opened a PR against your branch (avats-dev#1). That PR doesn't change the main logic, it just adds one test and expands the other two a bit, so that we have good test code to work against when validating the logic in the main function. (By the way, if you're interested in this approach to development, I recommend reading up on Test Driven Development.)

Augment and strengthen tests for deprecation of edit parameter

avats-dev · 2020-09-19T13:38:03Z

@mdickinson , Why tests are passing on python 3.9?

mdickinson · 2020-09-21T07:10:01Z

Why tests are passing on python 3.9?

Probably because these tests aren't running on Python 3.9: we don't set up the GUI system on Python 3.9, since there are issues with the PySide2 back end.

If you look at the detailed test run results on Python 3.9, you'll see that the tests in this PR are skipped:

test_edit_not_given (traits.tests.test_configure_traits.TestConfigureTraits) ... skipped 'TraitsUI not available'
test_edit_when_false (traits.tests.test_configure_traits.TestConfigureTraits) ... skipped 'TraitsUI not available'
test_edit_when_true (traits.tests.test_configure_traits.TestConfigureTraits) ... skipped 'TraitsUI not available'

traits/has_traits.py

mdickinson · 2020-10-05T09:57:10Z

@avats-dev It would be great to get this finished up and merged. Is there anything I can help with?

mdickinson · 2020-10-13T13:35:12Z

@avats-dev I made a new PR (#1311) that builds on your commits to fix this issue. Thank you for your contributions here!

avats-dev · 2020-10-19T13:41:18Z

@mdickinson sorry I couldn't complete this. Will contribute more when I'm free. Thanks for the PR.

mdickinson · 2020-10-22T12:00:56Z

@avats-dev No problem; thank you for the contribution! Note that my PR built on yours, so your commits did go into the master branch - you're now officially a Traits contributor.

avats-dev added 2 commits August 13, 2020 10:50

deprecated edit argument to configure_traits

598df46

Check for deprecation warning

b53e4f6

avats-dev added 5 commits September 6, 2020 05:40

fixed edit arg deprecation

c35e11b

fix flake8 error

1c506c5

fix docstring markup error

fb4b16e

fix explicit markup error for docs

7cdc76d

remove blankspace

a299ca2

mdickinson reviewed Sep 7, 2020

View reviewed changes

traits/has_traits.py Show resolved Hide resolved

mdickinson reviewed Sep 7, 2020

View reviewed changes

mdickinson requested changes Sep 7, 2020

View reviewed changes

mdickinson mentioned this pull request Sep 7, 2020

Intermittent failure in CI (traits.tests.test_regression) #1282

Closed

added spaces and refcatored

8d708d4

mdickinson reviewed Sep 17, 2020

View reviewed changes

traits/has_traits.py Outdated Show resolved Hide resolved

refactor again

e553c4e

avats-dev requested a review from mdickinson September 17, 2020 17:15

Expand tests slightly, add explicit test for case where edit not passed

cea80d3

Merge pull request #1 from enthought/deprecate-edit-additional-testing

aaf0bbd

Augment and strengthen tests for deprecation of edit parameter

mdickinson reviewed Sep 22, 2020

View reviewed changes

traits/has_traits.py Show resolved Hide resolved

mdickinson mentioned this pull request Oct 13, 2020

CLN: Deprecate the edit parameter to configure_traits #1311

Merged

2 tasks

mdickinson closed this Oct 13, 2020

avats-dev deleted the feature/deprecate-edit branch October 19, 2020 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a DeprecationWarning for when edit is false in configure_traits() #1272

Add a DeprecationWarning for when edit is false in configure_traits() #1272

avats-dev commented Aug 13, 2020 •

edited

kitchoi commented Aug 13, 2020

kitchoi commented Aug 13, 2020 •

edited

avats-dev commented Aug 13, 2020

avats-dev commented Aug 18, 2020

kitchoi commented Aug 18, 2020

kitchoi commented Aug 18, 2020

avats-dev commented Aug 18, 2020

mdickinson commented Sep 1, 2020

avats-dev commented Sep 1, 2020 •

edited

avats-dev commented Sep 6, 2020

mdickinson commented Sep 7, 2020

mdickinson Sep 7, 2020 •

edited

avats-dev Sep 17, 2020

mdickinson Sep 17, 2020

mdickinson left a comment

mdickinson commented Sep 18, 2020

mdickinson commented Sep 18, 2020

avats-dev commented Sep 19, 2020

mdickinson commented Sep 21, 2020 •

edited

mdickinson commented Oct 5, 2020

mdickinson commented Oct 13, 2020

avats-dev commented Oct 19, 2020

mdickinson commented Oct 22, 2020

Add a DeprecationWarning for when edit is false in configure_traits() #1272

Add a DeprecationWarning for when edit is false in configure_traits() #1272

Conversation

avats-dev commented Aug 13, 2020 • edited

kitchoi commented Aug 13, 2020

kitchoi commented Aug 13, 2020 • edited

avats-dev commented Aug 13, 2020

avats-dev commented Aug 18, 2020

kitchoi commented Aug 18, 2020

kitchoi commented Aug 18, 2020

avats-dev commented Aug 18, 2020

mdickinson commented Sep 1, 2020

avats-dev commented Sep 1, 2020 • edited

avats-dev commented Sep 6, 2020

mdickinson commented Sep 7, 2020

mdickinson Sep 7, 2020 • edited

Choose a reason for hiding this comment

avats-dev Sep 17, 2020

Choose a reason for hiding this comment

mdickinson Sep 17, 2020

Choose a reason for hiding this comment

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson commented Sep 18, 2020

mdickinson commented Sep 18, 2020

avats-dev commented Sep 19, 2020

mdickinson commented Sep 21, 2020 • edited

mdickinson commented Oct 5, 2020

mdickinson commented Oct 13, 2020

avats-dev commented Oct 19, 2020

mdickinson commented Oct 22, 2020

avats-dev commented Aug 13, 2020 •

edited

kitchoi commented Aug 13, 2020 •

edited

avats-dev commented Sep 1, 2020 •

edited

mdickinson Sep 7, 2020 •

edited

mdickinson commented Sep 21, 2020 •

edited