Fix #1021 by updating View constructor to convert state as dict to class object #1022
Conversation
seratch
added
bug
| self.values = value_objects | ||
|
|
||
| def to_dict(self, *args) -> Dict[str, Dict[str, Dict[str, dict]]]: # type: ignore | ||
| self.validate_json() | ||
| if self.values: # skipcq: PYL-R1705 | ||
| if self.values is not None: |
There was a problem hiding this comment.
This has been a potential bug - {} is a falsy value.
| @@ -35,7 +35,7 @@ | |||
| "callback_id": "view_4", | |||
| "external_id": "some-unique-id", | |||
| "state": { | |||
| "values": [] | |||
| "values": {} | |||
There was a problem hiding this comment.
This has been a potential issue of test data. The values usually cannot be an array.
| @@ -30,6 +30,7 @@ def setUp(self) -> None: | |||
| def verify_loaded_view_object(self, file): | |||
| input = json.load(file) | |||
| view = View(**input) | |||
| self.assertTrue(view.state is None or isinstance(view.state, ViewState)) | |||
There was a problem hiding this comment.
This assertion used to be failing before the change. Before the change in this PR, the passing one is self.assertTrue(view.state is None or isinstance(view.state, dict)).
| @@ -110,6 +111,18 @@ def test_valid_construction(self): | |||
| ), | |||
| ), | |||
| ], | |||
| state=ViewState( | |||
There was a problem hiding this comment.
Just in case, added test pattern to pass a ViewState object, not a dict.
| self.values = value_objects | ||
|
|
||
| def to_dict(self, *args) -> Dict[str, Dict[str, Dict[str, dict]]]: # type: ignore | ||
| self.validate_json() | ||
| if self.values: # skipcq: PYL-R1705 |
There was a problem hiding this comment.
As we no longer use the code analyzer, we can remove this type of comment.
seratch
force-pushed
the
issue-1021-view-state-init
branch
3 times, most recently
from
fddee14
to
ce98cc0
Compare
Codecov Report
@@ Coverage Diff @@
## main #1022 +/- ##
==========================================
- Coverage 86.65% 83.66% -2.99%
==========================================
Files 95 95
Lines 8789 8792 +3
==========================================
- Hits 7616 7356 -260
- Misses 1173 1436 +263
Continue to review full report at Codecov.
|
…ct to class object
seratch
force-pushed
the
issue-1021-view-state-init
branch
from
ce98cc0
to
291b432
Compare
|
To: reviewers, let me know if you have any comments. |
|
Let me merge this one but feel free to write comments if you have any |
Summary
This pull request fixes #1021 by updating the
slack_sdk.models.views.View's constructor to convert a givenstatedict value to theViewStateclass object.As all other properties in
Viewclass are already converted to class objects, changing this part would be a great improvement in terms of consistency. The changed property is still compatible with the current type hint -Optional[Union[dict, ViewState]]. We are going to apply this change in the next minor version - 3.6.Category (place an
xin each of the[ ])/docs-src(Documents, have you run./docs.sh?)/docs-src-v2(Documents, have you run./docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python setup.py validateafter making the changes.