Merged
Conversation
3c2178a to
2b024e5
Compare
2b024e5 to
6a8c67f
Compare
sbrodehl
reviewed
Oct 21, 2022
Collaborator
sbrodehl
left a comment
There was a problem hiding this comment.
Thanks for the PR , it looks like a very nice idea!
For now, I have two (more general) comments:
- DFS: Should the recursive DFS be improved right away?
- lambda expression: Should the AST be parsed and checked, instead of manual search?
| self.fn = fn | ||
| self.fn_src = getsource(self.fn) if self.fn is not None else None | ||
|
|
||
| if self.fn_src is not None: |
Collaborator
There was a problem hiding this comment.
Why is the code checked manually, instead of parsing the complete expression and creating the AST, looking for variable usage?
Collaborator
There was a problem hiding this comment.
If have implemented the dependency checking via the python AST, see miniflask/dev-ast branch.
Collaborator
There was a problem hiding this comment.
Feel free to merge #122, then we can merge this afterwards?
Split from "fixup! API-change: refactor of state-dependency handling"
…) if x in state and [...] else expr
10c0a03 to
205f07c
Compare
205f07c to
fe4d23d
Compare
Fix order of deps, now always sorted. Add more test cases. Remove unused argument 'state'. tests: Add test for failing stuff. Split from "Remove 'meta' inspection."
…commit is pushed.
Inspect register() call to maybe get more insights. Remove 'meta' inspection. Ensure single expression is found in source. Define and use private members. Fix string search for 'state' argument. Remove not allowed statements. Remove unused import. Remove empty lines. Add noqa to class. Spam noqa. fjgruiewhjfiogbhpa fjgruiewhjfiogbhpa Disable more warnings. Fix typo.
Owner
Author
|
Great work here, @sbrodehl. Thanks a lot for helping me out for this tough implementation! 🚀 |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Graph-Based Dependency Evaluation
Attention: This is a breaking change.
This MR changes the way dependencies are defined, checked & evaluated.
Description
Previous implementation for dependency solving has been done for very simple cases, where no loops or harder dependencies could be parsed.
This MR changes that by refactoring the whole registration process and the evaluation of the variables.
Breaking API-Changes
lambda state, event: ...to be used for state-registrations.**This PR changes the way lambda expressions are parsed and evaluated.
mf.likeunnecessary, thusmf.likehas been removed from miniflask completelyNew Behavior
This MR implements graph based dependencies.
To accomplish this, it does the following during registration:
mf.state_registrationscontains all information of all variable registrations.state_node-instances. This new class wraps all the information needed for graph-based dependency evaluation and checks without ever saving the actual data. This is to ensure also large amounts of data to be saved instate-variables without every worrying about unintentional copies in the interpreter.(I have taken special care to not change this).
a. First we look for the last registration for every variable, taking the usual order (see 2.) into account.
b. Next we check the graph for circular dependencies or unresolved dependencies (using a simple DFS-routine). In case something is found, all errors are shown at once.
c. Using the same routine from (b.), we sort all variables into an order that can be parsed linearly.
d. Next, we evaluate all variables for the first time taking their dependencies into account.
e. Then we can define the cli-argument parser using the values of the first pass (d.).
f. Finally, we match the cli-arguments to the actual state-variables (unchanged) and re-evaluate the graph to take overwrites into account.
Summary / Things done in this MR
mf.like-API fromminiflask.tests/state/dependencies[settings]to match the new APIThe test checked for values before the
mf.run, i.e. the first event that is called. The new implementation does not require to "remember" the initial state until the user-parameters are read from cli, which was just a work-around in the old system.Check all before creating this PR:
Examples
The following list shows all types of lambda expressions supported with this MR**:
Notes on Lambda-Parsing
Checking for state-calls is rather reliable in one-liners. The harder part is to find out which variables can be omitted, because the user wishes to have one variable out of a list.
I have solved this here by specifically allowing lambda expressions of a predefined form,
allowing also multiple variables to be used, i.e.
cond1 and cond2 and cond3 and ....All this is mainly to minimize the dependencies.
In case the source-code matching of the if-else-expression above fails, all state-calls will be used as a dependency.