Ad
  • Custom User Avatar
    • Fixed outstanding issues from previous forks
    • Rewrote random tests:
      • Tests are now split into separate groups of valid and invalid expressions
      • Invalid expressions are just valid expressions with random invalid bits inserted
      • These are generated deterministically, which means we no longer need a reference solution
      • While I'm not totally happy with how random expressions are generated, this is how it's done in other languages and I can't be bothered to rewrite that just for this translation. If it works, it works.
  • Custom User Avatar

    There are still outstanding issues that you left unadressed. But what's more, I realized that the random string generator just produced a long Str[Normal()...] 2/3 of the time, and the rest it managed to produce an invalid string. These aren't interesting inputs. The entire setup (even in the source language) is just flawed. Basically, what we want are random expressions that are either valid or invalid, but otherwise look similar.

    For this reason, and to avoid having to go back and forth many more times, I took the liberty of forking my own revision to apply the fixes I deem necessary.

    You're free to look over it and comment on it; after all, this is still your translation and you might have issues with changes I made. Otherwise I will approve it at some later time.

  • Custom User Avatar

    I'm not sure how to parse your message. Here's what I typically do:

    assert_eq!(actual, expected, "\nYour result (left) did not match the expected output (right) for the input: {input:?})

  • Custom User Avatar

    Would this be an okay asserion message?

    Left - actual result, right - expected result. Test failed on string: "{case_str}"

  • Custom User Avatar
  • Custom User Avatar
    • You may have misunderstood my comment about the mod preloaded part. For clarity, I'll just post here what the test setup should look like:
    mod preloaded;
    
    #[cfg(test)]
    mod tests {
      use crate::preloaded::RegExp;
      use super::str_to_regex;
      // other use statements needed by test suite
    
      mod solution {...}
    
    • The struct RegExp snippet is still in the initial solution. Remove it, and replace it with the statement use preloaded::RegExp
    • On that note, the description snippet is missing a space: enum RegExp{
    • Thanks for adding assertions, but they are incomplete. You need to clarify tot he user what "left" and "right" mean (they don't know, they can't see the assertion). Also, notice how you're repeating the same assertion code over and over in every test. Making a change means modifying multiple places. Be DRY: Move all of it into its own function, and just call that function instead of asserting in each test. Make the function accept a string as input (the test case) and an expected value. The function computes the user's solution from the string and compares it to the expected value with a nice assertion.
    • In the first example you still have a Box::new, despite saying you're omitting it:
    // Box::new() parts are ommited from Or and ZoreOrMore variants for readability
    "ab*"    -> Str(vec![Normal('a'), ZeroOrMore(Box::new(Normal('b')))])
                                                 ^^^^^^^^
    
    • By the way, it's spelled "omitted", not "ommited".
    • impl Display for RegExp belongs in Preloaded, together with the struct itself.
    • In your random_regexp generator, your Normal case produces characters in the entire printable range. This includes * and |, which is very likely not what you want. Again, just write a constant byte array b"abcd...ABCD..." and sample that randomly (you need rand::seq::SliceRandom)

    As before, this needs more fixes. Please have another go at it.

  • Custom User Avatar

    Wow! Thank you for that review and recommendations!

  • Custom User Avatar

    Notes:

    • Your reference solution (in mod solution) can leak to the user. Please move it under mod tests, and change pub fn _str_to_regex to pub(super) fn...
    • On a related note, please give your reference solution a name that more clearly disambiguates it from the actual user solution (not just a prefix _) so test code is easier to review.
    • All assertions need proper assertion messages. I would recommend using assert!(...) and constructing a custom message that displays the input and clearly points out what is expected and what was actually returned by the user. Always test your assertions with a purposefully failing solution. You might find it useful to create a custom testing function that takes an input string, an expected result, and does the comparison and proper assertions in one place, and use that everywhere you are currently calling assert_eq!.
    • There is no need to put the preloaded code in the initial solution; the interface is described in the solution, and that is sufficient.
    • Move mod preloaded into the tests section (both sample and full tests), at the very top (outside mod tests), but leave use preloaded::RegExp in the initial solution. The reason is that if a user chooses to modify code (e.g. removing the mod statement), then this should only affect their code (it will break), but should not affect test code. use super::RegExp is exactly what we don't want to rely on in the tests module.
    • I would suggest modfying the examples in the description: add a comment explaining that Box::new is ommitted for readability, then go ahead and actually remove all the noisy Box::new stuff. This should make the structure much more clear at a glance (the necessity for boxing is an implementation detail). After all, the sample tests contain the Box::new calls, so unfamiliar users should understand how to create recursive structures from that.
    • In the sample tests, I would suggest formatting large nested ASTs in such a way that their structure is more easily discerned (add new lines and indentation, similar to what you'd get with {:#?} printing). Readability is valuable, line-count is not.
    • I trust that your reference solution is correct, so I won't review that. But it seems your random tests only test random strings. Other languages I can see also construct and test random ASTs, presumably because those are all guaranteed to be valid. Please add those too. The Python translation (which I assume was the basis of this one) has a template that you can copy. But note that you are always free to come up with your own implementation that might be more idiomatic, as long as it produces correct results that cover all required cases.
    • On a technical note, your method of constructing a random string is perhaps not the most intuitive or efficient (via chars().nth()). I'd recommend turning printable (which is a silly name, taken from Python) into a byte slice (b".."), then using SliceRandom::choose length times: (0..length).map(|_| printable.choose(&mut rng).unwrap() as char).collect()

    Because the translation as is contains critical issues, I will reject it. Please feel free to fork it and apply any necessary fixes, so it can be reviewed again.

  • Custom User Avatar

    This comment is hidden because it contains spoiler information about the solution

  • Custom User Avatar