Controllable task refactoring#784
Conversation
justonedev1
left a comment
justonedev1
left a comment
There was a problem hiding this comment.
I approve, but I added some small details that can be addressed or not, depends on you.
However, it took my quite a lot of time because there were multiple things combined (as you mentioned): splitting code into functions, extracting common logging fields but also changing of error handling around pollTaskForStandbyState. However, as I couldn't be sure that there weren't some changes on the way I had to read through the code in basically two editors, one with master and one with changes.
So maybe for future refactoring it might be better to either create different PRs (imho excessive) or one PR with different commits and tell me when one kind of changes is commited (eg. splitting of Launch function) at time. I can have a quick look mark it as approved in my PR and github will remember it and only show changes when another commit is applied to the same PR and I would only see the changes from commit to commit and it would be much clearer and faster.
Commits can be squashed and merged/rebased to master when PR is done.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
After learning more about github you can discard my previous comment. I thought that you can comment and go through only latest commit and not all of them. |
we reduce boiler-plate by having a common set of log fields that are defined once. infologger.level constant is used instead of "level". "cmd" and "command" log fields were unified to "command".
Launch is broken into smaller pieces for better readability. Some minor error handling bugs are automatically fixed on the occasion and some more are marked with fixme and will be taken care of in separate commits.
knopers8
force-pushed
the
executor-error-reporting
branch
from
77a7e8d to
16a0e5d
Compare
|
feedback applied, merging to move on with more fixes |
2 participants
Some boiler-plate of logging is reduced, then Launch() is split into smaller chunks. Some bugs were identified during the process, they will addressed in future commits.