[CX-1181] Feat: lazychain by hermansje · Pull Request #48 · datacamp/protowhat

hermansje · 2019-10-15T16:45:36Z

This PR builds on #42, so the full diff includes those changes as well.

Main work:

Refactor SCT syntax to be an execution graph and enable derived features
Refactor exceptions to share an interface to simplify logic
Refactor feedback to be centralized and built from formalized components
Add autodebug

codecov · 2019-10-15T16:46:51Z

Codecov Report

Merging #48 into master will increase coverage by 0.9%.
The diff coverage is 88.04%.

@@            Coverage Diff            @@
##           master      #48     +/-   ##
=========================================
+ Coverage   81.42%   82.32%   +0.9%     
=========================================
  Files          16       18      +2     
  Lines         845      956    +111     
=========================================
+ Hits          688      787     +99     
- Misses        157      169     +12

Impacted Files	Coverage Δ
protowhat/utils.py	`100% <ø> (ø)`	⬆️
protowhat/checks/check_logic.py	`94.54% <100%> (-0.2%)`	⬇️
protowhat/checks/check_files.py	`100% <100%> (ø)`	⬆️
protowhat/Test.py	`84.61% <100%> (-2.06%)`	⬇️
protowhat/sct_context.py	`100% <100%> (ø)`
protowhat/checks/check_funcs.py	`41.46% <11.76%> (+0.51%)`	⬆️
protowhat/checks/check_bash_history.py	`95.65% <50%> (-2.13%)`	⬇️
protowhat/Reporter.py	`96.36% <87.5%> (-2.15%)`	⬇️
protowhat/State.py	`84.37% <87.87%> (-0.82%)`	⬇️
protowhat/failure.py	`88.46% <88.46%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1e621d...d34913b. Read the comment docs.

TimSangster · 2020-01-16T07:39:10Z

+    # Then
+    assert len(sct_dict) == 3
+    assert sct_dict["has_chosen"] == check_simple.has_chosen
+    assert sct_dict["success_msg"] == check_simple.success_msg


Why not check the third one?

TimSangster

Looks like a clear improvement. I think the following is still needed for maintainability:

Add more documentation (especially to sct_syntax and sct_context)
Improve test coverage even more
Find a way to split up sct_syntax further

But this is no reason to block progress.

vvnkr

I approve, but do have some remarks.

Currently, there are 48 commits. Can we squash these into 4 commits, related to:

Refactor of feedback
Refactor of F to LazyChain
Debugging logic
Add xwhat embeddings

If that's too much work, I'm fine with not doing it... It's just something I'd do.

vvnkr · 2020-01-20T13:41:41Z

+    """
+    Create the globals that will be available when running the checks for the embedded technology.
+
+    Extra keyword arguments are passed to the constructor of the State for the embedded technology.


Are they? Where?

This used to be true, now the only extra arg is derive_custom_state_args, whose return value is used as kwargs when constructing the state.
I will swap **kwargs by derive_custom_state_args=None in this function and refer to create_embed_state in the docstring.

vvnkr · 2020-01-20T17:52:25Z

+            msgs = msgs[-3:]
+
+        # format messages in list, by iterating over previous, current, and next message
+        for prev_d, d, next_d in zip([None, *msgs[:-1]], msgs, [*msgs[1:], None]):


d stands for?

I copied this from the old method on State, it stands for 'dict' I think.

normal callables should also work

TODO: decide on consistent property name

the removed path is no longer called