Fixes #5402: State domain refactor #5505
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5505 +/- ##
========================================
Coverage 45.71% 45.71%
========================================
Files 504 504
Lines 29751 29751
Branches 4502 4502
========================================
Hits 13602 13602
Misses 16149 16149Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks, @kevintab95! This is a great start. I left some comments, PTAL. But overall it looks pretty clean!
core/domain/exp_domain.py
Outdated
| @@ -31,6 +31,7 @@ | |||
| from core.domain import html_validation_service | |||
| from core.domain import interaction_registry | |||
| from core.domain import param_domain | |||
| import core.domain.state_domain as state_domain | |||
There was a problem hiding this comment.
Ah, no, could you please do "from core.domain import state_domain", same as all the others?
There was a problem hiding this comment.
Used import that way because of a cyclic dependency problem. Moving the constituents of state to state_domain solved it, thanks!
core/domain/state_domain.py
Outdated
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Domain object for state.""" |
There was a problem hiding this comment.
Ah, we should put all the constituents of state here as well. I.e. Interaction, AnswerGroups, etc. -- basically, everything that a state consists of.
(Please modify the fileoverview docstring accordingly.)
| @@ -0,0 +1,543 @@ | |||
| # coding: utf-8 | |||
There was a problem hiding this comment.
Shouldn't there be tests for this file?
There was a problem hiding this comment.
Yep, I'll push tests for this tomorrow. Thanks!
There was a problem hiding this comment.
Moved relevant tests from exp_domain_test to state_domain_test. Can you PTAL @seanlip? Thanks!
core/domain/state_services.py
Outdated
| @@ -0,0 +1,45 @@ | |||
| # coding: utf-8 | |||
There was a problem hiding this comment.
Shouldn't there be tests for this file?
core/domain/state_services.py
Outdated
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Commands that can be used to operate on states""" |
There was a problem hiding this comment.
Add period at end.
That said, I think you can get rid of this file, and have a classmethod on the State object instead. Something like State.convert_state_dict_to_yaml(). Then use cls.from_dict(state_dict) within it.
|
Hi @kevintab95. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
|
Well, it looks almost nice -- Travis tests are failing... |
Explanation
Fixes #5402: Moved state class from exp_domain to state_domain. Also, moved related functions to state_services.
Checklist
python scripts/pre_commit_linter.pyandbash scripts/run_frontend_tests.sh.