-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/15 gen random #25
Conversation
…e like the erratic js tool that it will need to. doesn't build.
…rait of Grammar. Still exploring strategies of the implementation.
…erflow and updating the docs
…ort nonterminals without their own productions
Added logic to accept the double quote character as a valid terminal character by allowing both single and double quotes to delimit terminals. Added test to generate a random valid BNF grammar from the grammar for BNF itself.
… generate to return an error rather than an option probably and the testing needs more thought
Prior to this commit from_str was an alias for another trait called from_parse. This commit removes from_parse so there is only from_str. It exists as a custom trait of each node so the user isn't required to `use std::str::FromStr` but that trait is implemented for each node as well so a user may use these types in generic functions that require that trait.
README.md
Outdated
to evaluate it will print the identifer as a nonterminal, i.e. `<identifier>`. | ||
|
||
The generate function will return an error if it detects an infinite loop caused | ||
by a production such as `<PATTERN> := <PATTERN>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:=
or ::=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
README.md
Outdated
let grammar = Grammar::from_str(input); | ||
match grammar { | ||
Ok(g) => println!("{:#?}", g), | ||
Err(e) => println!("Failed to make grammar from String: {:?}", e), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do bnf errors impl display? If not, they should. If they already do, then just no debug :?
is necessary. Is the debug easier to read than the impl display?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no they shouldn't need debug, it's just a bad habit I picked up to default to always use :?
src/error.rs
Outdated
|
||
#[cfg(test)] | ||
mod tests { | ||
// use super::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a clarifying comment or a leftover?
pub fn productions_iter(&self) -> Iter { | ||
Iter { | ||
iterator: self.productions.iter(), | ||
} | ||
} | ||
|
||
/// Get mutable iterator of the `Grammar`'s `Productions`s | ||
/// Get mutable iterator of the `Grammar`'s `Production`s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
src/grammar.rs
Outdated
} | ||
|
||
let nonterm = Term::Nonterminal(ident.clone()); | ||
let mut result = String::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a small point, but what do you think about moving result
down to where it is used? As I was reading the code I had assigned some mental effort to tracking its mutations and was surprised to find none until the very end.
src/grammar.rs
Outdated
/// Ok(s) => println!("random sentence: {}", s), | ||
/// Err(e) => println!("something went wrong: {}!", e) | ||
/// } | ||
/// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc string example made me realize that for Expression::remove_term
or w/e the example hides the extern
/use
/main
etc. If we want that context in these small examples, then that one should be updated to match this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I kind of like it, I'll update it
src/grammar.rs
Outdated
/// Err(e) => println!("something went wrong: {}!", e) | ||
/// } | ||
/// } | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe overkill, but can you think of any hidden assert
s at the end of this example that would help prevent it from going stale in the future? If not, then no worries, not a must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give an example? assert what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///# assert!(sentence.is_ok())
or something would be enough for me. Wouldn't show up in the docs, but would be another canary if somehow the example gets messed with in the future and no longer parses or generates successfully.
src/grammar.rs
Outdated
/// ``` | ||
pub fn generate(&self) -> Result<String, Error> { | ||
let start_rule: String; | ||
let lhs = self.productions_iter().nth(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is the first production, not the lhs as the name suggests. And some of the name confusion continues in the match below, referring to the production as a term.
revised to the correct '::='
This PR represents the addition of a feature to generate random sentences from an existing grammar object with a
generate
trait. Thegenerate
trait returns a result in order to handle problematic grammars and report what happened.Documentation was updated as well as the test suite.
Closes #15