Skip to content

Commit

Permalink
add type information for variable element in XML
Browse files Browse the repository at this point in the history 
this information is needed to distinguish between various varaible types
like function or plain value.
  • Loading branch information
@jreidinger
jreidinger committed Mar 5, 2013
1 parent 86e68b9 commit 94c2902
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion libycp/src/YExpression.cc
View file
Expand Up @@ -149,6 +149,7 @@ YEVariable::toXml( std::ostream & str, int /*indent*/ ) const
{
str << "<variable name=\"";
str << m_entry->toString (false /*definition*/);
str << "\" type=\"" << m_entry->catString();
return str << "\"/>";
}

Expand Down Expand Up @@ -1946,7 +1947,7 @@ YEBuiltin::toStream (std::ostream & str) const
std::ostream &
YEBuiltin::toXml( std::ostream & str, int indent ) const
{
str << "<builtin name=\"" << StaticDeclaration::Decl2String(m_decl) << "\"";
str << "<builtin name=\"" << m_decl->name << "\"";

if (m_parameterblock != 0)
{
Expand Down

7 comments on commit 94c2902

@mvidner
Copy link
Member

@mvidner mvidner commented on 94c2902 Mar 5, 2013

Choose a reason for hiding this comment

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

I think you may be breaking the well-formedness of the result:

$ xmllint angle-in-attr.xml
angle-in-attr.xml:2: parser error : Unescaped '<' not allowed in attributes values
<variable name="angle_brackets" type="list<string>" />
                                          ^

Also, why the change?

@jreidinger
Copy link
Member Author

Choose a reason for hiding this comment

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

@mvidner you are right builtin change is mistake that should not go here. I revert this part.
About xml validness how you get such type? catString should not return it - https://github.com/yast/yast-core/blob/master/liby2/src/SymbolEntry.cc#L214

@jreidinger
Copy link
Member Author

Choose a reason for hiding this comment

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

revert is fixed in @5fc2aa0

@mvidner
Copy link
Member

@mvidner mvidner commented on 94c2902 Mar 5, 2013

Choose a reason for hiding this comment

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

Ah, I see. So I'd expect the attribute to be named category, because type of a variable has an established meaning already. But it also depends on how other instances of catString are serialized.

@jreidinger
Copy link
Member Author

Choose a reason for hiding this comment

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

@mvidner you are right, that is exactly also my idea what I think about it during yesterday night. So change in 4a6f03f

@mvidner
Copy link
Member

@mvidner mvidner commented on 94c2902 Mar 6, 2013

Choose a reason for hiding this comment

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

Good!

Hm, this discussion under individual commits is hard to follow. For the record, this has been an ex-post code review after an accidental push.

I think that next time, if little time has passed since we notice the mistaken push, we should simply warn on IRC and force-push the previous HEAD.

@jreidinger
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, force pushing is not acceptable for me, especially when we also have automatic build submissions on jenkins, I don't hesistate to show in git history that I make sometimes mistakes :)

Please sign in to comment.