Skip to content

Use context properties instead of state #191

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.

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

Merged
merged 16 commits into from
Jul 12, 2018
Merged

Use context properties instead of state #191

merged 16 commits into from
Jul 12, 2018

Conversation

remo5000
Copy link
Contributor

@remo5000 remo5000 commented Jul 11, 2018

Features

  • Uses Context's chapter property instead of a sourceChapter property in the state
  • Moved assessmentId check into AssessmentWorkspace
  • Uses Context's isRunning to check isRunning

Issues fixed

Fixes #39

Directly following

  • Make workspace reset accept a library parameter

@coveralls
Copy link

coveralls commented Jul 11, 2018

Pull Request Test Coverage Report for Build 283

  • 14 of 23 (60.87%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 26.011%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/assessment/AssessmentWorkspace.tsx 8 9 88.89%
src/containers/PlaygroundContainer.ts 0 1 0.0%
src/containers/academy/grading/GradingWorkspaceContainer.ts 0 1 0.0%
src/containers/assessment/AssessmentWorkspaceContainer.ts 1 3 33.33%
src/sagas/index.ts 1 5 20.0%
Files with Coverage Reduction New Missed Lines %
src/reducers/workspaces.ts 1 13.75%
src/sagas/index.ts 1 27.36%
Totals Coverage Status
Change from base Build 275: 0.3%
Covered Lines: 701
Relevant Lines: 2285

💛 - Coveralls

remo5000 added 3 commits July 11, 2018 18:58
As of now, interruption of runtime works, but the stop button does not
get reset. I suspect that this is because of the order of calling
between saga vs the reducers.
This will allow for 2 calls -- one for the interpreter to actually stop
the program and one to call after interruption, to cause UI changes
@remo5000 remo5000 changed the title [WIP] Use context properties instead of state Use context properties instead of state Jul 11, 2018
@remo5000 remo5000 requested a review from ning-y July 11, 2018 11:18
...state[location],
isRunning: false
}
...state
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, there is a chance that this fails due to its async nature.

interrupt(context) in the saga inserts an interrupt error into the context. The scheduler only stops on the next evaluation step when it comes across this interrupt error. Then, the scheduler stops execution and sets context.isRunning to false. END_INTERRUPT_EXECUTION depends on the scheduler having done all this before the reducer takes this action of recreating the state, so that the props has a new value for isRunning and therefore triggers a re-render of the eval/run/stop buttons.

Instead, make END_INTERRUPT_EXECUTION explicitly (and redundantly) set isRunning in the context to false, and also push an interrupt error into the context (reference).

* Called to signal the end of an interruption,
* i.e called after the interpreter is told to stop interruption,
* to cause UI changes.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can we move these comments to the reducers? I think there are at least three places we can comment on the actions:

  • actionTypes
  • actionCreators
  • reducers & saga

And I think we should standardise them being in the reducers and saga because

  1. Most of them are already there, and
  2. That's where the logic is

@remo5000
Copy link
Contributor Author

Thanks, changes made 😄

@ning-y ning-y merged commit 7a2c087 into master Jul 12, 2018
@ning-y ning-y deleted the use-context-chapter branch July 17, 2018 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement interrupt/stop using context.runtime.isRunning?
3 participants