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

configurable view deriver #2021

Merged
merged 71 commits into from
Apr 10, 2016
Merged

configurable view deriver #2021

merged 71 commits into from
Apr 10, 2016

Conversation

digitalresistor
Copy link
Member

This is a work in progress PR, this branch we want to use to continue pushing changes until this feature is complete and ready to go.

Things left to do:

  • Merge master into branch and then continue development
  • Make tests pass fully, including coverage
  • Change view, default, **kw to view, value, **kw. We want to pull the value out of the kwargs automatically and set it as the value, if there is no value in the kwargs we pass in the registered default.
  • Change **predicates to **view_options, and split out **predicates and **options. Options is going to hold on to the kwargs for the derivation views, and predicates is going to hold on to the kwargs for the predicates, as the name implies.
  • auto-call @wraps_view when registering the view derivation using add_view_derivation. We want to keep @wraps_view as private API until someone says they absolutely need it.
  • outer_derivation = attr_wrapped, predicated_view. These have to exist, without them the whole view machinery does not work. So we don't allow them to be replaced/changed/or anything.
  • inner_derivation = -renderered_view, we need to register rendered_view as a default, but we don't want to dictate that no-one could potentially insert a view derivation after it.
  • Update tests to use view, value, **kw rather than view, default, **kw
  • Documentation for all of the view derivation stuff
  • Update request processing diagram

The Future Possibly Holds:

  • make config._add_predicate register the discriminator twice, just in case someone used the discriminator for the view predicate, currently it is renamed to view options. While a b/w incompatible change one that @mmerickel and I agree doesn't really hurt. We will only do this if absolutely necessary.
  • Add option name to add_view_derivation, so that you can define which kwarg you want, rather than basing it strictly on your view derivations name.

caseman and others added 30 commits April 15, 2015 10:06
Remove unneed config command in test.
This is required for future changes, where we need the list of predicate
names.
We also use the previously defined names() method on PredicateList to
fetch the list of predicates and pull out all predicate values into it's
own dictionary.

Then the rest of the values are the view options for the view
derivations. At the moment we do not check if the options matches a
valid view derivation. No seatbelts for view options.
This is an integration test in the sense that it adds both a view
derivation and then adds a view. That view is then tested to verify that
it correctly returns and that the derivations haven't broken anything.
This way we can use that in tests.
Specifically predicated_view and attr_wrapped_view exist for MultiView
to function, without them the view machinery doesn't work. They aren't
technically view derivations because they are just containers for some
data that the view machinery requires.
@mmerickel
Copy link
Member

I just took care of point 2.... "over" now means closer to ingress, which is the same terminology used by tweens.

@digitalresistor
Copy link
Member Author

While I agree that the tween API has its limitations, I don't think nor want that to hold up merging this new API in its current form just because the two API's are dissimilar. I do think we should allow maybe_dotted for the view derivers, but passing in a class or function or anything along those lines is fine as well. I don't think limiting view derivers is the right thing to do.

We should start another PR to discuss how to fix the tween issue.

@stevepiercy
Copy link
Member

Docs and docstrings LGTM (subject to successful Travis-CI builds)!

@digitalresistor
Copy link
Member Author

Opened a new ticket for the Tween configuration. I really don't want it holding up these changes because I am already using view derivations using this branch for a semi-production app :P

@mmerickel
Copy link
Member

Okay my last issue is some UX / book-keeping stuff.

Tweens do a bit of work to ensure that over/under are strings and also provide pyramid.tweens.INGRESS, pyramid.tweens.EXCVIEW and pyramid.tweens.MAIN APIs for easily adding things over/under. config.add_tween also does some work to avoid adding things over ingress and under main.

Currently it's basically wrong to ever call config.add_view_deriver without specifying BOTH over and under arguments. Without both you have very minimal guarantees on where the deriver will appear. The only case that is handled properly is the default which will place the tween over rendered_view and under decorator_view.

The main thing that worries me is that if you specify under='decorated_view' there is no guarantee that the deriver will appear before rendered_view. This will be a bug if your deriver is depending on the response being a response object as the api changes after egress of rendered_view.

Anyway I don't really have any action items about this but I want to bring it up as an easy way to shoot yourself in the foot. One option is to always specify over/under if they are None, forcing a user to override them. Envision def add_view_deriver(config, name, over='rendered_view', under='decorated_view'). This leaves the defaults in a good spot and at least it will always be over rendered_view unless you change it. The main foot-gun here is that you will get a cyclic dependency error if you specify under='rendered_view' without overriding over='rendered_view' to something - and there is actually no API right now to define that something because we don't have MAIN or INGRESS so we'd have to define them.

I swear this is my last issue.

@mmerickel
Copy link
Member

I don't really have a problem leaving things as-is but here's what I'm talking about. If a deriver does not define both an over and an under then the deriver is underspecified and could be placed into unexpected spots in the view pipeline. For example if I say under=authdebug_view then the deriver could end up being sorted under rendered_view which has a different contract for the return value (context, request) -> any_python_obj versus (context, request) -> response. While the deriver may sort properly in the developers app it may not work in someone else's app depending on the other derivers added.

The feature I've been considering is simply a sanity check step after registering all derivers that would ensure that every deriver eventually maps back to one of the builtins in both over and under directions. It would be a breaking change down the road to require derivers to be fully specified which is why I was thinking about it now. Note that tweens currently suffer from the problem I'm describing as well.

@mmerickel mmerickel mentioned this pull request Mar 22, 2016
16 tasks
@digitalresistor
Copy link
Member Author

I vote for making users fully specify their over/under and anything else raises an error. None of this wishy washy type stuff that leads to potential errors down the road.

@digitalresistor
Copy link
Member Author

👍 I am very happy with this branch!

@mmerickel
Copy link
Member

I'm also happy with it. The only other thing I can think of changing atm is some checks to force preserving the order of the builtins but I don't think it's a big deal.

@mmerickel mmerickel merged commit b6187b0 into master Apr 10, 2016
@mmerickel mmerickel deleted the feature/configurable-view-deriver branch April 10, 2016 17:49
@digitalresistor
Copy link
Member Author

Thank you @mmerickel for all your hard work on making this a succes :-)

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.

5 participants