Fork twiddle should save changes#134
Conversation
|
LGTM |
|
Wait, got a bug. |
|
@stefanpenner OK, now you can review. |
There was a problem hiding this comment.
What is the correct way to do this?
There was a problem hiding this comment.
since this is async:false toArray() should work, instead of currentState (if im reading this correctly). This should also prevent leaking the internalRecord in the forEach and likely should squash both private API usages.
|
there appears to be very API related code in the route, it might be nice to extract that. |
|
@stefanpenner Hey, the ‘very api related code’ uses private API. I was wondering if there was a correct, public, way to do that. |
There was a problem hiding this comment.
extracting the error callback into its own function, so it can be united tested will help cleanup this code some more. The extra verbosity distracts from the overall flow of logic.
|
Again, it wasn't working correctly, so I took another approach. |
|
@stefanpenner Updated. Please review again. |
|
Ideally you would keep the closure actions and move the actions off the router and into a service. |
|
@mmun Perhaps, but that should be a separate PR. Can you open an issue? |
…into fork_should_save
|
@joostdevries @stefanpenner @rwjblue Can I get a review? |
|
LGTM – |
Fork twiddle should save changes
4 participants
Fixes issue #133