On Tue, Sep 7, 2010 at 12:03 PM, Markus Roberts <[email protected]>wrote:

> > +        # Skip things that respond to :instantiate (classes, nodes,
> > +        # and definitions), because they have already been
> > +        # instantiated.
> > +        if !child.respond_to?(:instantiate)
> >           child.safeevaluate(scope)
> >         end
>
> I think you have an indentation error here.


I believe this is a diff artifact.  If you look at
http://github.com/stereotype441/puppet/blob/ticket/next/4685/lib/puppet/parser/ast/astarray.rb,
the indentation looks correct.


> > +# statements is like statements_and_declarations, but it doesn't allow
> > +# nested definitions, classes, or nodes.
> > +statements: statements_and_declarations {
> > +  val[0].each do |stmt|
> > +    if stmt.is_a?(AST::TopLevelConstruct)
> > +      error "Classes, definitions, and nodes may only appear at toplevel
> or inside other classes"
> > +    end
> > +  end
> > +  result = val[0]
> > +}
> > +
>
> Wouldn't it be simpler / easier / clearer to leave statements as is and
> define statements_and_declarations in terms of statements and declarations?
> E.g., let the grammar handle it rather than dynamically enforcing a
> grammatical rule by checking the AST as it's built?
>

Sure, that would work too.  However, the advantage of this approach is that
we get to control the exact error message the user receives if they try to
use a class, definition, or node where they shouldn't.  If we do it in the
grammar, then the error message will be "syntax error at 'classname';
expected '}'", and it may require some ingenuity on the part of the customer
to figure out how to fix that.  I think it is especially important to give
the user a clear error message in this case, since previous versions of
Puppet permitted these constructions (albeit with misleading semantics).

I just realized, however, that the patch as I submitted it isn't ideal
either: it produces the error message at the end of the block of statements
that contains the forbidden declaration, rather than on the line number of
the forbidden declaration itself.  I'll submit a follow-on patch to correct
that.

Paul

-- 
You received this message because you are subscribed to the Google Groups 
"Puppet Developers" 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/puppet-dev?hl=en.

Reply via email to