Skip to content

Simplify module field evaluator#1697

Closed
bobot wants to merge 2 commits intomasterfrom
simplify_module_field_evaluator
Closed

Simplify module field evaluator#1697
bobot wants to merge 2 commits intomasterfrom
simplify_module_field_evaluator

Conversation

@bobot
Copy link
Copy Markdown
Collaborator

@bobot bobot commented Dec 20, 2018

Reformulate the checks so that they are easier to check.

Signed-off-by: François Bobot <francois.bobot@cea.fr>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@rgrinberg
Copy link
Copy Markdown
Member

@emillon mind reviewing this?

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Dec 20, 2018

Sure! But it probably won't be before tomorrow.

@rgrinberg
Copy link
Copy Markdown
Member

I ended up having time today after all.

Code looks very good and is one of the few cases where the operators increased the readability. I merged this with a few tweaks to the error messages and decluttering find_errors by hoisting up all the helper functions to Properties. Thanks Francois.

@rgrinberg rgrinberg closed this Dec 20, 2018
@rgrinberg rgrinberg deleted the simplify_module_field_evaluator branch December 20, 2018 21:45
@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Dec 21, 2018

OK! I'm not convinced by the operators by I'm willing to try :)

@bobot
Copy link
Copy Markdown
Collaborator Author

bobot commented Dec 21, 2018

Thanks! Is there a drawback for you to force push your modification then merge? I think it helps external people know which features are merged.

@rgrinberg
Copy link
Copy Markdown
Member

There's no drawback. I will force push and just merge via github.

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.

3 participants