Skip to content
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

[CX-1182] Feat: lazychain #389

Merged
merged 19 commits into from
Jan 28, 2020
Merged

[CX-1182] Feat: lazychain #389

merged 19 commits into from
Jan 28, 2020

Conversation

hermansje
Copy link
Member

@hermansje hermansje commented Nov 20, 2019

Update pythonwhat to build on this protowhat PR.

@hermansje hermansje changed the title Feat: lazychain [CX-1182] Feat: lazychain Jan 14, 2020
@hermansje hermansje self-assigned this Jan 14, 2020
@hermansje hermansje marked this pull request as ready for review January 14, 2020 13:24
Copy link

@vvnkr vvnkr left a comment

Choose a reason for hiding this comment

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

No big remarks here; don't forget to squash!

@@ -35,6 +40,7 @@ def __len__(self):
return len(self._items)


@parameters_attr
Copy link

Choose a reason for hiding this comment

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

As discussed in person, this feels a bit error-prone. However, I'm fine with just being careful with it every time a *what needs to extends the ProtoState.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added some reasoning behind this decorator in the docstring of parameters_attr in protowhat to make it easy to understand why this was done in the future (when we want to think of a solution that eliminates this trade-off).

Copy link
Member Author

@hermansje hermansje Jan 20, 2020

Choose a reason for hiding this comment

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

To have the parameters attribute value available on the whole State class hierarchy and its instances without recalculation or decorator (or a manual list), State in protowhat could get a metaclass with a parameters property that memoizes the value.
I chose this because it's easier to understand, but one metaclass in protowhat would prevent having to think about it in xwhats.

class F(ProtoF):
def __init__(self, stack=None, attr_scts=sct_dict):
super().__init__(stack, attr_scts)
def Ex(state=None):
Copy link

Choose a reason for hiding this comment

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

Why not:

Ex = ExGen(sct_dict, State.root_state)

?

Copy link
Member Author

@hermansje hermansje Jan 20, 2020

Choose a reason for hiding this comment

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

This causes trouble with dill when using the manual converter feature.
I'll clarify the last commit message a bit:

Dilling a custom converter function causes globals to be dilled too.
The root_state attribute on ChainStart instances (e.g. ExGen(...)) includes things that can't be dilled.

The manual converters are being phased out.

dilling a custom converter function causes globals to be converted
the root_state attribute on ChainStart instances includes things that can't be dilled
@codecov
Copy link

codecov bot commented Jan 28, 2020

Codecov Report

Merging #389 into master will decrease coverage by 0.19%.
The diff coverage is 86.13%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #389     +/-   ##
=========================================
- Coverage   94.18%   93.98%   -0.2%     
=========================================
  Files          26       26             
  Lines        2115     2128     +13     
=========================================
+ Hits         1992     2000      +8     
- Misses        123      128      +5
Impacted Files Coverage Δ
pythonwhat/test_funcs/test_object_accessed.py 100% <100%> (ø) ⬆️
pythonwhat/feedback.py 100% <100%> (ø)
pythonwhat/test_exercise.py 97.67% <100%> (+0.17%) ⬆️
pythonwhat/checks/check_object.py 100% <100%> (ø) ⬆️
pythonwhat/utils_ast.py 86.36% <100%> (-0.6%) ⬇️
pythonwhat/test_funcs/test_function.py 100% <100%> (ø) ⬆️
pythonwhat/test_funcs/test_compound_statement.py 96.55% <100%> (ø) ⬆️
pythonwhat/checks/check_function.py 100% <100%> (ø) ⬆️
pythonwhat/test_funcs/utils.py 78.33% <33.33%> (+1.28%) ⬆️
pythonwhat/checks/check_has_context.py 94.28% <66.66%> (-2.86%) ⬇️
... and 9 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 a2805eb...223ec8a. Read the comment docs.

@hermansje hermansje merged commit d01d2a3 into master Jan 28, 2020
@hermansje hermansje deleted the jh/lazychain branch January 28, 2020 18:35
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.

2 participants