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

support engine options overrides by output type #240

Merged
merged 3 commits into from
Mar 11, 2013

Conversation

mojavelinux
Copy link
Member

  • allow engine options to be overridden for specific output types
  • move default options under 'default' key
  • set default options for haml and slim
  • merge data from default site yaml when loading site yaml

- allow engine options to be overridden for specific output types
- move default options under 'default' key
- set default options for haml and slim
- merge data from default site yaml when loading site yaml
@mojavelinux
Copy link
Member Author

It's not enough to have tilt options per engine. Options are sometimes dependent on whether HTML or XML is being generated by the template. This change allows default options to be set per engine and extension and option overrides to be set by the output extension.

We may need to discuss this change to see if we like the result. I had to introduce a level of nesting in the engine options and some merge logic.

Instead of:

slim:
  :format: :html5

we now have

slim:
  default:
    :format: :xhtml
  html:
    :format: :html5

The 'default' key is required or else the output specific options get caught up in the options hash.

@LightGuard
Copy link
Member

Feels like we should have some tests for the merge logic

@mojavelinux
Copy link
Member Author

There's at least one indirectly, but I'll agree it's weak.

I think we should also recurse so that the merge logic goes to an arbitrary depth. I just needed something working to get started.

How do you feel about the extra level of config? Any suggestion of an alternative?

@LightGuard
Copy link
Member

It seems like it's something that we'll deal with but users probably won't. Will users need to add an extra level in their site.yml to override?

@mojavelinux
Copy link
Member Author

Yes. The reason the extra level is required is because you have to be explicit about what output the config applies to. The default node is required because otherwise you would send invalid options to Tilt.

For example, if you had:

slim:
  :format: :html5
  xml:
    :format: :xhtml

Then if you were pulling the default options, you would get 'xml' as an option going to tilt. The alternative would be to filter those nodes out, but that is just messy.

Other options would be:

slim:
  :format: :html5
slim|xml:
  :format: :xhtml

That would avoid the nesting and perhaps make things simpler for the users since the default node is now gone. WDYT?

@mojavelinux
Copy link
Member Author

...and no, slim is not smart enough to realize that you don't want :html5 as a format when the doctype is XML. Such is life.

@mojavelinux
Copy link
Member Author

I kind of like the flat layout.

@LightGuard
Copy link
Member

This works for me.

On Mon, Mar 11, 2013 at 10:07 AM, Dan Allen notifications@github.comwrote:

I kind of like the flat layout.


Reply to this email directly or view it on GitHubhttps://github.com//pull/240#issuecomment-14722169
.

Jason Porter
http://en.gravatar.com/lightguardjp

LightGuard added a commit that referenced this pull request Mar 11, 2013
support engine options overrides by output type
@LightGuard LightGuard merged commit ecd0470 into awestruct:master Mar 11, 2013
@mojavelinux
Copy link
Member Author

Wait, were we going to go with the flat layout? I interpreted your response as a need for an updated pull request. I can do another if we want to change it.

@mojavelinux mojavelinux deleted the per-output-engine-config branch March 11, 2013 18:33
@LightGuard
Copy link
Member

I didn't see a flat layout, seemed like you had one, when I didn't see it
figured you'd dropped the idea so I merged.

On Mon, Mar 11, 2013 at 12:33 PM, Dan Allen notifications@github.comwrote:

Wait, were we going to go with the flat layout? I interpreted your
response as a need for an updated pull request. I can do another if we want
to change it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/240#issuecomment-14731580
.

Jason Porter
http://en.gravatar.com/lightguardjp

@mojavelinux
Copy link
Member Author

We just got mixed up. No worries, I opened an issue and we'll go from there.

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.

None yet

2 participants