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

Simplate work #481

Merged
merged 21 commits into from
Aug 13, 2015
Merged

Simplate work #481

merged 21 commits into from
Aug 13, 2015

Conversation

pjz
Copy link
Contributor

@pjz pjz commented Jul 30, 2015

Get rid of is_bound and make all simplates require at least three sections, per #297

@chadwhitacre
Copy link
Contributor

@pjz I've updated our labels a bit to fit the rest of Gratipay (nixed 0 - Whatever and 2 - Blah, which were empty anyway; renamed 1 - Ready to Ready to Start and 3 - Done to Review). When this is ready, apply the Review label, and it will show up in our Gratipay-global Review queue. :-)

@pjz pjz force-pushed the simplate_work branch 2 times, most recently from db2c58b to a7129ad Compare July 30, 2015 15:57
@pjz pjz mentioned this pull request Jul 31, 2015
@@ -0,0 +1,5 @@

# for backwards compatibility with aspen-renderer modules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or should we update all of the renderers along with this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Baby steps. Let's not break backward compatibility just yet. Esp. Since we might be breaking simplates out entirely.

@chadwhitacre
Copy link
Contributor

@pjz I'm applying Review to this, ya?

@pjz
Copy link
Contributor Author

pjz commented Aug 10, 2015

Yes. Sorry, forgot about that.

@pjz
Copy link
Contributor Author

pjz commented Aug 11, 2015

Note this also fixes #417, as a website object is no longer needed to instantiate a simplate.

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

I get the feeling @whit537 has been avoiding this review.

@chadwhitacre
Copy link
Contributor

I would say "warming up to it." ;-)

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

po-TAY-to, po-TAH-to...

@chadwhitacre
Copy link
Contributor

!m @pjz

@@ -8,7 +8,7 @@
from __future__ import unicode_literals

from . import Renderer, Factory
from .. import json
from ... import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have aspen.json? Weren't we going to move that to aspen.simplates.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the json registry; It'll move, I just haven't gotten there yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe even to aspen.simplates.renderers.json_dump?

The concept of `state` belongs to algorithm.py, which shouldn't leak
over into simplates.
@pjz
Copy link
Contributor Author

pjz commented Aug 13, 2015

uhh... WTF? why are you doing this work here instead of merging my PR and then doing it in new PRs?

@chadwhitacre
Copy link
Contributor

Yeah, yeah, sorry, got carried away. I made a new PR at #490 and force-pushed the last three commits off of here, but I maybe clobbered 1aec879?

@chadwhitacre
Copy link
Contributor

(@pjz You okay with d5d48d2, at least?)

@pjz
Copy link
Contributor Author

pjz commented Aug 13, 2015

It's okay. Yeah, I repushed 1aec879. d5d48d2 is fine.

@chadwhitacre
Copy link
Contributor

Cool.

@chadwhitacre
Copy link
Contributor

We can't get a test for the _ordinal call?

@chadwhitacre
Copy link
Contributor

Other than that I think I just need to grok https://github.com/gratipay/aspen-python/pull/481/files#r36929962.

@pjz
Copy link
Contributor Author

pjz commented Aug 13, 2015

There's as many tests for _ordinal as there were for what it replaced (which was a Class with a fancy magic handler, IIRC). I'm really not sure what to test with it; it's a straightforward mapping function.

@chadwhitacre
Copy link
Contributor

Well, what we want to test is that it's called properly from wherever we had _ordinal[foo]. No?

@pjz
Copy link
Contributor Author

pjz commented Aug 13, 2015

Well, where we originally had ORDINALS[foo], actually, but yes.

@pjz
Copy link
Contributor Author

pjz commented Aug 13, 2015

That was limited to this file, because it's declared here and not imported anywhere else. I think it's actually limited to the four times in this block (twice per error message).

@chadwhitacre
Copy link
Contributor

Well, where we originally had ORDINALS[foo], actually, but yes.

I guess we didn't have a test for it before this PR. Out of scope!

chadwhitacre added a commit that referenced this pull request Aug 13, 2015
@chadwhitacre chadwhitacre merged commit 100854d into master Aug 13, 2015
@chadwhitacre chadwhitacre deleted the simplate_work branch August 13, 2015 03:02
@chadwhitacre
Copy link
Contributor

!m @pjz

@chadwhitacre
Copy link
Contributor

Follow-ons: #490, #491.

Changaco pushed a commit that referenced this pull request Mar 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants