I think empty mixin defs shouldn't be an error - it might be useful to have a mixin that doesn't do anything.
I've only got a couple more very minor nitpicks: - Calling a mixin include a "directive" is sort of confusing terminology. A directive in Sass already means an @-statement (import is currently the only example). - A couple lines of the patch add extra trailing whitespace. It would be nice if they didn't. - The convention I tend to use for newlines in comments and documentation is adding newlines on sentence or sub-sentence breaks, rather than just when I reach a certain line length. This makes the comments easier to go back and edit in the future without having to re-align everything. It would be cool if your comments did that, too. - I think it would be more clear if the "Invalid mixin name" error was instead "Undefined mixin." Again, these are very small issues. - Nathan On Fri, Apr 4, 2008 at 10:54 AM, Garry Hill <[EMAIL PROTECTED]> wrote: > > > Hi, > > On 4 Apr 2008, at 17:48, Nathan Weizenbaum wrote: > > > * Mixin declarations should only be allowed at the root of documents, > > like constants and @import directives. This restriction doesn't seem > > to > > be in place. > > * Why is that last exception test commented out? > > * "echo -e '-a\n c: d' | sass" dies with an unsightly error. > > The following patch rolls up all others and includes the fixes and > tests that answer the above issues. > > http://pastie.org/175395 > > There was a kinda deep issue with the parsing of the mixins that did > all sorts of weirdness when you had a mixins with no whitespace > between them. I think that's what was throwing the error with '-a\n > c: d'. > > The commented test was one I couldn't get to work, which I think was > also a symptom of the error i describe above. So that's now working. > There is one commented exception test that looks at empty mixin defs, > e.g. "-empty\n\n". I'm just not sure that this is needed so I've left > in the test and the code to pass it if it's required, your call. > > g > > > > On 4 Apr 2008, at 17:48, Nathan Weizenbaum wrote: > > > > > This is much better. There are a few issues I've noticed, though: > > > > * Mixin declarations should only be allowed at the root of documents, > > like constants and @import directives. This restriction doesn't seem > > to > > be in place. > > * Why is that last exception test commented out? > > * "echo -e '-a\n c: d' | sass" dies with an unsightly error. > > > > Thanks for all your work! I'm getting more and more fond of this > > feature. > > - Nathan > > > > Garry Hill wrote: > >> Hi, > >> > >> Here's the latest version: > >> > >> http://pastie.org/175298 > >> > >> > >> On Apr 2, 8:44 pm, Nathan Weizenbaum <[EMAIL PROTECTED]> wrote: > >> > >>> There shouldn't be a Tree::MixinNode type. It's just semantically > >>> wrong. > >>> I also don't like #append_to. This should be implementable without > >>> touching anything in tree/ at all. > >>> > >>> Really, I'd prefer duplicating a little appending code to kludges > >>> like > >>> this. You could even avoid the duplication by making some sort of > >>> append_all_children method. > >>> > >> > >> No more MixinNode. > >> > >> I think it's pretty clean, despite my earlier worries about code > >> repetition. > >> > >> One thing I was looking at was being able to skip the current > >> returning of a string on mixin include and instead simply return an > >> array of nodes, which led me to this follow up patch: > >> > >> http://pastie.org/175310 > >> > >> The only real extra change, outside of tweaks to things I'd already > >> tweaked in the previous patch, was to return an array of > >> DirectiveNodes instead of ValueNodes from the "imports" method, which > >> is actually more consistent with the DirectiveNode that's returned in > >> the case of a raw CSS import. > >> > >> Oh, Thomas, yes, the code you described works as expected. It and > >> even > >> more complex examples have made it into the test template. e.g. > >> > >> -deep > >> a:hover > >> :text-decoration underline > >> > >> .deep > >> +deep > >> > >> == > >> > >> produces > >> > >> .deep a:hover { > >> text-decoration: underline; > >> } > >> > >> === > >> > >> I think this has really profound implications that I hadn't even > >> thought about when I started hacking this in. Fab. > >> > >> Best, > >> > >> g > >> > >> > >> > >>> > >> > >> > > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Haml" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/haml?hl=en -~----------~----~----~----~------~----~------~--~---
