Introduce explicit I/O namespace#467
Conversation
2300ab4 to
73481f8
Compare
5a81b5b to
47fe843
Compare
doc/user/guide/parse.rst
Outdated
| SeQuant supports creating expression objects by parsing an appropriately formatted input string. This process is referred to as *parsing*. | ||
| Furthermore, SeQuant can also serialize expression objects into this string representation, which is referred to as *deparsing*. | ||
|
|
||
| The functions for performing these tasks are :func:`parse_expr(…)`, :func:`parse_result_expr(…)` and :func:`deparse(…)` respectively. The former two |
There was a problem hiding this comment.
is this a good time to revisit the "incosistent" naming?
deparseis similar toto_latexbut names are very different ... how aboutto_sqtextorto_sqmathdeparseis seemingly only a word used in R language (i.e. it's not an actual word)parse_exprandparse_result_exprare factories,make_exprandmake_result_exprwould be more in line with naming of factories in SQ and elsewhere ...
There was a problem hiding this comment.
If we do make changes, now would indeed be a good moment.
I kinda like the symmetry between the parse and deparse function names. We don't really have that with the alternate names you suggested...
Also, given that what we do is parsing, I would prefer to actually have parse in the name 👀
There was a problem hiding this comment.
I kinda like the symmetry between the
parseanddeparsefunction names. We don't really have that with the alternate names you suggested...
I like the symmetry too. I like design precision even more.
Can keep it, but not in the top-level namespace. The issue with these names (and the reason for this PR) is that these must be specific to a format that they refer to (and with parse_* function we must declare explicitly what we are constructing). So we must namespace them. So perhaps what we need is:
sequant::io::latex::parse...;
sequant::io::latex::deparse; // does this exist? At some point deparse actually took pure latex as input ...
sequant::io::sqtext::v1::parse...;
sequant::io::sqtext::v1::deparse;
sequant::io::sqtext::v2::parse...;
sequant::io::sqtext::v2::deparse;
To avoid such long names we still need top-level sugar, such as to_latex. I propose to_text for sqtext::default::deparse and from_text<{Expr,ResultExpr}> as top-level sugar.
Also, given that what we do is parsing, I would prefer to actually have parse in the name 👀
Parsing is an implementation detail, name should reflect what users care about (to_text, from_text ... how these are implemented? They should not care).
There was a problem hiding this comment.
so perhaps this is what I have in mind:
sequant::io::latex::to;
sequant::io::latex::from...;
sequant::io::sqtext::v1::to;
sequant::io::sqtext::v1::from...;
sequant::io::sqtext::v2::to;
sequant::io::sqtext::v2::from...;
using namespace sequant::io::sqtext::default = sequant::io::sqtext::v1;
sequant::to_latex = sequant::io::latex::to;
sequant::to_text = sequant::io::sqtext::default::to;
etc.
There was a problem hiding this comment.
I like the idea of an io namespace that bundles all different input/output formats. I would probably use function names serialize and deserialize instead of to and from though. To me that makes the function calls appear a bit more natural… I guess with to and from I would kinda expect the format to be attached as to_latex and from_latex rather than latex::to and latex::from.
However, since not all of these formats are intended to be roundtrip-capable (e.g. LaTeX only works in the expression-to-string direction afaict) serialize is probably not quite correct (though I think we could use serialize instead of sqtext). So maybe using to_string and from_string instead is the way to go?
Something like
sequant::io::latex::to_string;
sequant::io::serialization::to_string; //Note: these are actual implementations that do the forwarding at runtime
sequant::io::serialization::from_string;
sequant::io::serialization::v1::to_string;
sequant::io::serialization::v1::from_string;
sequant::io::serialization::v2::to_string;
sequant::io::serialization::v2::from_string;
// Note sure if we really should define these (users can bring the functions in their namespace as desired)
sequant::to_latex = sequant::io::latex::to_string;
sequant::serialize_to_string = sequant::io::serialization::to_string;
sequant::deserialize_from_string = sequant::io::serialization::from_string;There was a problem hiding this comment.
I like the idea of an
ionamespace that bundles all different input/output formats. I would probably use function namesserializeanddeserializeinstead oftoandfromthough. To me that makes the function calls appear a bit more natural… I guess withtoandfromI would kinda expect the format to be attached asto_latexandfrom_latexrather thanlatex::toandlatex::from.
I realized as I was typing the previous message that the meaning of to/from was ambiguous ... latex::to is producing string (like to_latex)? etc. But serialize/deserialize can be misconstrued similarly.
How about format[::versionX]::{to,from}_[result_]expr?
then sequant::to_latex becomes sequant::io::latex::from_expr.
sequant::io::serialization::to_string; //Note: these are actual implementations that do the forwarding at runtime
Fine w/ it.
eec7765 to
50c761e
Compare
50c761e to
a43724d
Compare
b230c6e to
0f47709
Compare
Also, remove some redundant deparse() overload declarations (they are implicitly accessible via the overload for Expr &).
Without optimizations, these tests can take forever and hence hamper running tests repeatedly during development cycles.
Support was incomplete anyway and since we don't have intentions on completing it, we remove it in order for nobody to rely on it.
At the same time, switch to consistently using toUtf8/toUtf16
0f47709 to
daec090
Compare
See individual commits.
TODO:
wstring.hppintoutility/string.hppRemove strayto_stringoverloads - support for that is quite inconsistent and user's should prefer using explicit I/O functionsionamespace for all text<->object conversions