From: <and...@am...> - 2003-05-28 18:31:51
|
> Not keeping blancs at parsing time or adding some when > writing to indent is breaking the XML specifications. > However I think it can be useful to do so. I agree that it is useful to do so - sufficiently useful that some such functionality should be provided as part of a standard library like libxml++ (or else it will be constantly reinvented in slightly different ways). > So I propose to modify the API of libxml to give the > possibility not to keep blanks (xmlKeepBlanksDefault option in libxml). However, I question whether it is appropriate to add options to the parsing functions. That's a slippery slope. If more options get added this way it leads to "fragile base class", which matters to me because I hope/plan to have my own Parser derived class, something like TwigParser (halfway between DomParser and SaxParser). Better to make it a property... since C++ doesn't really have properties, make it a data member of Parser, with the appropriate accessors. > 1) add a parameter to parsing functions : > Document* > xmlpp::parse_file( > const std::string& filename, > bool keepblanks = KeepBlancs::Default > ) > throw(exception) > etc. Q: I'm a little bit confused by this. I thought that parse_file, etc., were methods on a parser object, e.g. xmlpp::Parser::parse_file, xmlpp::DomParser::parse_file, xmlpp::SaxParser::parse_file. Has it been made into a free function in a version I have not downloaded yet? (I wish I could get CVS across my company's firewall, legitimately.) Anyway, assuming that it has not been: Make your whitespace handling mode a private data member of Parser. With the appropriate accessors. > 2) add an accessor to Parser : > Parser::set_keepblanks(bool value) A slight trick, from Stroustrup, that gives you the equivalent of keyword options: Instead of void Parser::set_keepblanks(bool value) make it Parser& Parser::set_keepblanks(bool value) as in Parser& Parser::set_keepblanks(bool value) { this->m_keepblanks = value; return *this; } So, instead of DomParser domparser; domparser.parse_file(filename,/*keepblanks=*/true); you do domparser.set_keepblanks(true).parse_file(filename); Such "Stroustrup keywords" have some minor issues: either you want to make parse_file a virtual function in Parser, so that it picks up the derived DomParser::parse_file or SaxParser::parse_file; or you have to make set_keepblanks a delegate in derived classes, with a slightly different return. {Virtual functions better, IMHO.} > 3) add following functions to Document : > Document::write_to_formated_file(const std::string& filename, const > std::string& encoding = std::string()) throw(exception); > Document::write_to_formated_memory(const std::string& filename, const > std::string& encoding = std::string()) throw(exception); First, spelling: write_to_formatted_file... Second, this caused me to notice the assymmetry: Parser::parse_file(string_filename), Parser::parse_memory(string), Parser::parse_stream(istream) Document::write_to_file(string_filename), Document::write_to_string(string) "Obviously" want write_to_stream(ostream). It also tends to suggest that what is wanted is a Formatter object, separate from the Document. Possibly an object that exposes both a Parser interface for input, and a Formatter interface for output, since much of the time the input and output keep blanks setting will be similar. Maybe "Printer" is a betteer name than "Formatter". What's an antonym for "Parser"? But that's gilding the lily - BDUF, Big Design Up Front. For now, Document::write_to_formatted_file is fine, with the assunmption being that Document will have copied the keepblanks setting (aka the Formatter object) from the Parser that created it. > The default behavior would remain the same. > > Do you agree on this ? Overall, yes, with the minor tweaks to the interface that I mention above. |