Skip to content

Refactor nodetransformers#73

Open
angelsanzn wants to merge 40 commits intonestorsalceda:masterfrom
angelsanzn:refactor-nodetransformers
Open

Refactor nodetransformers#73
angelsanzn wants to merge 40 commits intonestorsalceda:masterfrom
angelsanzn:refactor-nodetransformers

Conversation

@angelsanzn
Copy link
Copy Markdown
Contributor

I did this in order to better understand the AST transformation and try to express it more clearly in the code. I welcome your feedback - I know the global diff is terrible but it really goes step by step on each commit.

The only real changes are:

  • Not calling ast.fix_missing_locations more than once, at the end of the transformation (because doing otherwise seems to be unnecessary).
  • Verifying that hook "scopes" (that is, all and each) are provided. Currently, mamba doesn't do it, so writing with before.whatever results in trying to save a before_whatever hook to the example group, which fails. With these changes, that with statement would not be transformed.

This also adds tests for the particular transformations.

Ángel Sanz added 30 commits January 9, 2016 00:05
This marks that instance variable as private.
`when` can take values 'all' and 'each', which do indicate *when*
the hook is to be run, but so do 'before' and 'after'. What's
unique to 'all' and 'each' is that they provide the scope of
the hook: whether they affect each example or are run only at the
edges.
This makes it a bit clearer that this class only performs AST
transformation (it doesn't produce spec objects or anything)
and briefly explains what the output syntax is about.
…he expected structure

The 'relevancy' of a `with` statement is defined as whether it has the structure
of an example group, example or hook declaration. This makes the transformation
bail out early when encountering `with` statements not matching those schemas,
such as the `with patch.object(...)` in 'spec/mock_patch_example_spec.py'.

However, those `with` statements with the structure of an example group, example
or hook declaration but not actually being one of those are not detected.
Examples:

    ```
    with my_context_manager('this is not a spec heading'):
        ...
    ```

    ```
        with my_thing.not_a_hook:
            ...
    ```

That's exactly the reason why the last `return node` is (at least for now)
necessary (since, in these cases, the `name` is not any mamba identifier,
hence control flows out of all the `if`s).
…ps_processed,

and start at 0

Not all nodes are counted (e.g. hooks are not counted), so this name
was not right. In addition, there's no problem with starting at 0,
and that is more coherent with the new name.
This also extracts WithStatement, HookDeclaration,
AttributeLookupRepresentingHookDeclaration and MethodDeclarationCreator.

This is a first step towards breaking up MambaSyntaxToClassBasedSyntax
into smaller classes.

This adds tests for HookDeclarationToMethodDeclaration and refactors
the fixtures "loading infrastructure" a bit.

The only changes in behaviour are:
    - stop using ast.copy_location: the call to ast.fix_missing_locations
    in the ExampleCollector is enough.

    - ignore nodes of the form:
    ```
        with before.something_weird:
            pass

        with after.whatever:
            pass
    ```
        Previously, in these cases mamba tried to create hooks
        and failed to do so (KeyError in Example.hooks). Now it
        just leaves those nodes untouched.
This also extracts ExampleDeclaration, CallOnANameWhereFirstArgumentIsString,
Counter and two more custom exceptions.

This also adds tests for this transformation (example -> method).

This includes some minor renames as well.

The behaviour is the same, except avoiding the call to ast.copy_location.
The only behaviour change is not using ast.copy_location.
We don't really need so many concrete exceptions or their messages.
…houldNotBeTransformed

This also removes some exception messages because they are not useful for now.
…odeShouldNotBeTransformed

This also removes the remaining error messages in exceptions.
…cture

We don't need two so-specific exceptions for this.
This expresses intent, rather than implementation.
This also renames NodeShouldNotBeTransformed to NotARelevantNode,
because now the declarations clearly own that exception, and
they should know nothing about transformations.
Instead, a query method is made public. Only raise from
the higher-level HookDeclaration.
Instead, expose a query method. Only raise from the higher-level classes which
use them (ExampleDeclaration and ExampleGroupDeclaration).
Instead, expose a query method and only raise from the higher-level classes
which use them (the specific transformers).
Now transformer classes can (and should) be queried for their ability to
transform the node given to them in construction.

The rationale is that, since all with statements are to be processed
indiscriminately, instantiating a transformer which can't actually transform
that particular with statement is not something exceptional but something that
will happen all the time. Hence it should not be signaled using exceptions.
@angelsanzn angelsanzn force-pushed the refactor-nodetransformers branch from de7308f to c0e9789 Compare April 10, 2016 00:28
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.

1 participant