Open digama0 opened 2 years ago
Thanks Mario! As for issues on my contributions:
Database::print_grammar
,Database::print_formula
etc. look like debugging tools, they shouldn't be in the public API. If they are, they should have a return value or write to an impl Write or something.
Yes, they indeed are debugging tools. Ok with that change for Grammar
. For Formula
, this could be done on the proposed FormulaRef
:
Display
implementation which would print the formula in a readable format,Debug
implementation which would, in addition, print the formula tree in a LISP format like wceq ( cA cB )
Substitutions implements Index<&Label>, which uses an unnecessary indirection since Label is a 4 byte Copy type.
Fix in #34
Grammar::parse_formula takes a &mut dyn Iterator but it could take &mut impl Iterator
Fix in #34
Formula::build_syntax_proof has an unused A type parameter.
Fix in #34
outline
says it is not stable, so it should probably not be public at all.
Actually is it just as (un)stable as the rest, and I've also used it in a toy dependent project. I'd say we keep it like that! :-)
For the rest - some of them would be included in an API revamp around Database
as proposed in #24 .
Snippet
looks very interesting!
OutlineNode::get_name is unsound because OutlineNode::name is public
Fix in #56
Database::diag_notations is a weird API. types could be a config struct with boolean fields, and the output could be provided by a callback to allow for incremental rendering and doing whatever the user wants to do with the diagnostics.
The output of Database::diag_notations
is now indeed provided through a callback since #49, but the types
parameter has not been changed yet.
util
should not be public
Fix in #42
Why are
filename
,fnv
andregex
being re-exported?
Fix in #42
parser::as_str
is unsound and should at least be private
It is used in mm-web-rs
, metamath-vspa
, rumm
, and metamath-web
, that's all 4 current dependencies. I think we shall keep it public.
What's your opinion, @digama0 ?
Diagnostic
doesn't have docstrings for the variants.
Yes but are the names and the messages in to_snippet
not self-explanatory already?
In any case, this would be easy to add.
parser::as_str
is unsound and should at least be privateIt is used in
mm-web-rs
,metamath-vspa
,rumm
, andmetamath-web
, that's all 4 current dependencies. I think we shall keep it public.What's your opinion, @digama0 ?
Well yes, it is unsound but convenient so I'm not surprised it is used. To avoid breakage, I think it would be fine to replace the existing as_str
with one that does from_utf8().unwrap()
instead of from_utf8_unchecked()
, and those dependents can either make their own private version that does the unsound thing or just use the runtime check. We can also add statement_as_str()
type methods on getters that both retrieve a string and convert it to a &str
so that they can make use of the invariant to soundly use from_utf8_unchecked
. (Be careful that this is actually correct though; the parser doesn't just bail on non-ascii input, so non-ascii can end up in some data structures.)
Diagnostic
doesn't have docstrings for the variants.Yes but are the names and the messages in
to_snippet
not self-explanatory already? In any case, this would be easy to add.
Not quite; the main purpose of the docs on the variants would be to explain what the arguments to the variants are. Beyond that, copying the first line "short" message from the error report is probably sufficient.
as_str
Actually the definition of as_str
is
/// Transmutes this token into a Rust string.
#[must_use]
pub fn as_str(ptr: TokenPtr<'_>) -> &str {
std::str::from_utf8(ptr).expect("TokenPtr is supposed to be UTF8")
}
So it is not unsafe. It'd rather the other way around: it might be an optimization to use from_utf8_unchecked()
internally.
Diagnostic
doesn't have docstrings for the variants.
Ok, agreed.
@tirix Oh, if that is as_str
then I guess the problem has already been addressed at some point. I will mark it as done.
This is a collection of issues I notice while looking at the docs.
Database
has a new method which takes aDbOptions
which implementsDefault
, butDatabase
doesn't implDefault
.DbOptions
actually correct?Database::parse
has a complicated input type. The desired flexibility is better accommodated with animpl FileResolver
, and it should have a basic API that takes animpl Read
orimpl BufRead
.Database::parse_result
is a public method that returns a private object.Nameset::update
shouldn't be public? It can't be called fromDatabase::name_result
.LookupLabel
, then pull out the label and... do something undocumented that depends onSegmentSet
to get a statement.Frame
is unrecognizable compared to the MM spec. Most of the fields probably shouldn't be public, and it should have an actual API for getting the bits out.Diagnostic
doesn't have docstrings for the variants.Database::export
should output to animpl Write
rather than opening a file itself, and statements should be identified in a uniform way, e.g. byStatementAddress
rather than using&str
,Token
,Atom
,StatementAddress
andStatementRef
depending on the function.Database::statement
andDatabase::export
shouldn't need to mutate the database.Database::print_grammar
,Database::print_formula
etc. look like debugging tools, they shouldn't be in the public API. If they are, they should have a return value or write to animpl Write
or something.Database::diag_notations
is a weird API.types
could be a config struct with boolean fields, and the output could be provided by a callback to allow for incremental rendering and doing whatever the user wants to do with the diagnostics.Notation
could be swapped out forannotate_snippets::snippet::Snippet
Executor
should not be public. Also, why is there an async implementation in this crate? Usesmol
or something.export::export_mmp
should not be public, it should be a function onDatabase
.Formula::display
should not produce a string, it should be an object implementingDisplay
. That object could beFormulaRef<'_>
which bundles together the formula and references into theDatabase
, so thatFormula::iter
can also be supported.Substitutions
implementsIndex<&Label>
, which uses an unnecessary indirection sinceLabel
is a 4 byteCopy
type.Formula::build_syntax_proof
has an unusedA
type parameter.Grammar::parse_formula
takes a&mut dyn Iterator
but it could take&mut impl Iterator
outline
says it is not stable, so it should probably not be public at all.StatementRef
s. Not all statement kinds have all the data, and the API should be a guide to what has what.parser
has lots of public types that should not really be public, or should be public with private members.parser::as_str
is unsound and should at least be privateOutlineNode::get_name
is unsound becauseOutlineNode::name
is publicProofTree
is not stand-alone, it requiresProofTreeArray
, so it should probably be exposed asProofTreeRef
retaining a reference to the arrayProofTreePrinter
has too many public fieldsVerifyExpr
should not be publicFrame
has too many public fields, and in particularoptional_dv
is public but has a private typeutil
should not be publicProofBuilder::build
has a complicated typefilename
,fnv
andregex
being re-exported? I don't see any need to do this