Skip to content

wordy 1.4.0 #736

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

Merged
merged 4 commits into from
Nov 18, 2018
Merged

wordy 1.4.0 #736

merged 4 commits into from
Nov 18, 2018

Conversation

petertseng
Copy link
Member

No description provided.

Error object; since Rust was already using `Option` we need take no more
action.

exercism/problem-specifications#1334
@petertseng petertseng added sync/readme Keep a README in sync with exercism/problem-specifications sync/tests Keep a test suite/version in sync with exercism/problem-specifications labels Nov 18, 2018
result = evaluate(result, opr, operand(&t.remove(0)));
Some(result)
}).collect::<Vec<_>>();
if words.len() < 3 {
Copy link
Member Author

@petertseng petertseng Nov 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no test case that exercises this condition. I am considering whether to submit What is? as wordy 1.5.0

};
let mut words = words.split_at(3).1;
while !words.is_empty() {
let tmp = apply_op(result, words)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have liked to write (result, words) = apply_op(result, words)?; or something, but that is not valid. A shame.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was pointed out in #665 (comment) . I wonder if I should also consider using that function that takes &mut. Note though that there still remains the possibility that the function will cause an early return None (currently done by the ?), so there's a bit more to it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, destructuring assignment would be a really nice usability improvement for the language.

while t.len() > 1 {
result = evaluate(result, opr, operand(&t.remove(0)));
opr = operator(&t.remove(0));
if !c.ends_with('?') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no test case tests this line of code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason to keep it, then. This implements a parser stricter than the tests and readme require.

if !c.ends_with('?') {
return None;
}
let words = c.trim_end_matches('?').split_whitespace().map(|word| {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that if there were a test case along the lines of What is 1 + 2??? then that might raise questions as to whether that case is acceptable. This code would accept it.

Some(result)
}).collect::<Vec<_>>();
if words.len() < 3 {
return None;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I didn't have to do this separately, but words[0..3] will panic if words.len() < 3. Maybe something with iter().take(3), but then I hae to collect as well and it doesn't seem worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think words[..3] might not panic, being an unbounded range. Might be worth looking into, at least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Looks like it still does though. thread 'reject_empty' panicked at 'index 3 out of range for slice of length 0', libcore/slice/mod.rs:1932:5

Existing example was too lax with its error detection and I did not find
a sensible way to bolt it on incrementally. I was unfortunately required
to rewrite from scratch.

The new example does have the property of being fewer lines than the
previous one, if that helps...?

Closes #705
I unfortunately considered the existing example solution not quite up to
the task of doing this, so I was forced to write a new implementation
from scratch.

exercism/problem-specifications#1383
@petertseng petertseng merged commit d1d2dad into exercism:master Nov 18, 2018
@petertseng petertseng deleted the wordy branch November 18, 2018 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sync/readme Keep a README in sync with exercism/problem-specifications sync/tests Keep a test suite/version in sync with exercism/problem-specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants