Use deduced_required value for config.push #145
Conversation
oyvindeide
force-pushed
the
add_deduced_on_push
branch
from
c4df2a1
to
b4c8b46
Compare
tests/test_push_deduced_required.py
Outdated
| self.assertEqual(first.heroes, second.heroes) | ||
| self.assertEqual(sorted(first.villains), sorted(second.villains)) | ||
|
|
||
| def test_push_deduced_required(self): |
There was a problem hiding this comment.
There are already tests on push in test_layers.py. Should we move this test over in that file? Or potentially move all tests on push into this test?
There was a problem hiding this comment.
Might make sense to move it, my main problem is that this isn´t a failing test, although I first thought it would be.
There was a problem hiding this comment.
So just to clarify for me a bit, because containers can no longer be required, must the programmer add a custom validation to make sure they are specified?
There was a problem hiding this comment.
Yeah, that should be fixed first! In that respect it might make sense to move it to test_depreation_warning.py. You should be able to build on one of the tests there to verify that deduce_required also is active after a push?
There was a problem hiding this comment.
Will push an actual failing test in a sec
Depends on what the programmer is after... A defaulted, empty container will be specified for lists and dicts, and for named dicts all fields will be defaulted (unless some cannot be defaulted validly, in which case an error will occur). If you want a different behaviour than this, you will need to do custom validation at the moment. |
oyvindeide
force-pushed
the
add_deduced_on_push
branch
2 times, most recently
from
1b2f4a0
to
c14d231
Compare
|
Test is updated, will try to find a more suitable place for it. |
tests/test_push_deduced_required.py
Outdated
| basic_config = basic_config.push({"not_required": 10}) | ||
| self.assertTrue(basic_config.valid) | ||
|
|
||
| self.assertTrue(basic_config._deduce_required) |
There was a problem hiding this comment.
You'll need to test the consequence of this, not the deduce_required field itself. Pylint will not allow it.
There was a problem hiding this comment.
Right, well, am already doing that as the config would be invalid without the change. Think that should be enough.
My preference is |
oyvindeide
force-pushed
the
add_deduced_on_push
branch
from
c14d231
to
35a0fa3
Compare
oyvindeide
force-pushed
the
add_deduced_on_push
branch
from
35a0fa3
to
1322ff6
Compare
|
|
No description provided.