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

The Grand Re-Evaluation #96

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

The Grand Re-Evaluation #96

wants to merge 19 commits into from

Conversation

aiwenar
Copy link
Contributor

@aiwenar aiwenar commented Sep 17, 2018

..., or a better way of handling values

Improve how arguments to various directives are evaluated.

Fixes #95, fixes Connexions/cnx-recipes#604

How it works currently

Every directive which accepts evaluated values either has its own evaluating logic, or uses Oven.eval_string_value. This poses a number of problems:

  • Code evaluating values is re-implemented with very few changes in every function which accepts evaluated values. This in particular causes most CSS functions to be implemented three times.
  • Functions which accept a default value evaluate said value as string, which causes problems when used in content: directive, see for example Connexions/cnx-recipes#604.

Additionally since evaluation happens immediately when processing directives target-* functions can only be implemented by pretending to be a string (oven.TargetVal).

How it works with this PR

All directives now use a single shared value evaluator: expr.evaluate (through a convenient wrapper Oven.evaluate). CSS functions are now implemented only once, and instead of returning a concrete Python value return instead a typed value (css.Value): combination of a CSS type (css.Type) and some transparent value. To extract the concrete value from a typed value callers of expr.evaluate specify what type they expect, and values are converted to that type, if possible.

This solves problems such as Connexions/cnx-recipes#604, as now attr() is free to return either a css.String, when the attribute exists, or a css.DocumentFragment returned by the default value of content().

To solve the special status of target-*() functions, conversion from typed values to concrete values has been delayed until Oven.bake. This allows target-*() functions to be just evaluations of normal functions delayed until after the entire tree has been processed, and has an (partially) unintended consequence of making target-*() functions usable in every context in which their normal counterpart is. For example, first-letter(target-string(#ref, name)) or target-string(#ref, name, target-string(#other-ref, other-name)) are now correct expressions and work just as one would expect.

Additional changes this PR makes

To simplify parsing of CSS values a new class, css.Parser, has been introduced, and all current manual parsers rewritten using it. This has substantially simplified implementations of nearly all directives (just take a look at do_string_set).

Failed tests

  • inside: I don't know why this happens
  • default: In CSS Values and Units Module Level 4, attr() takes a qualified name as attribute name, and that's how I implemented it now. Previously however cnx-easybake accepted anything which evaluated to a string. If this change is intentional implementation of attr should be changed, if not, the test case.
  • string_set and unicode: These tests suffer from first-letter is an identity when applied to result of the string() function #95. I assume that means tests need to be corrected, but I restrained from doing so until someone can confirm this.
  • uuid: UUIDs are now randomly generated rather than mocked. I have not been able to find out why.

@aiwenar aiwenar requested a review from reedstrm September 17, 2018 17:52
@aiwenar aiwenar changed the title Grand Re-Evaluation The Grand Re-Evaluation Sep 17, 2018
@reedstrm
Copy link
Contributor

@aiwenar sorry, didn't see this request 4 days ago. Starting a review now, but based on the description alone, this is a massive improvement! Will get it reviewed, merged and run through the battery of regression tests w/ Helene ASAP.

@brittweinstein
Copy link

@helenemccarron can we close this?

@helenemccarron
Copy link
Contributor

I think @reedstrm was going to review, this would be useful to get this merged, I think but I do not know if we have the resources do so as of now. Is there a way to transfer knowledge so we can take advantage of this work>

@brittweinstein brittweinstein added this to the CNX Backlog milestone May 23, 2019
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.

first-letter is an identity when applied to result of the string() function
4 participants