Add "+" operator

[DrunkJon]

I implemented the BitOr Trait (| operator) for Term and Expression as described in #34. This allows syntax that mirrors actual BNF like: let exp= Term::Terminal::from_str("A") | Term::Terminal::from_str("B") | Term::Terminal::from_str("C")

• Term | Term creates an Expression that contains both Terms.
• Expression | Term and Term | Expression add the Term to the Expression.
• Expression1 | Expression2 clones all terms from Expression2 and adds them to Expression1

I made seperate methods for Expression and &Expression to prevent unnecessary clones as recommended in https://doc.rust-lang.org/core/ops/index.html

While testing, I noticed that two expressions are only counted as equal when their .terms are in the exact same order. This causes strange behavior like: a | Expression::from_parts(vec![b,c]) != Expression::from_parts(vec![a,b,c]) This should probably be a new Issue, but the implementation for Term | Expression would have to be changed if this is intentional.

[shnewto]

This is awesome! 🙌 🎉 🙏 ❤️

I'll be able to take a closer look this evening / tomorrow (I'm in MST) but in the meantime if you're able run cargo fmt on the code and commit what it changes, I think that should be enough to get through the format checks failing now.

Thank you!!

[codecov[bot]]

Codecov Report

Merging #88 (cee2b0f) into main (52d7599) will increase coverage by 0.38%. The diff coverage is 96.00%.

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   91.56%   91.94%   +0.38%     
==========================================
  Files          10       10              
  Lines         794      869      +75     
==========================================
+ Hits          727      799      +72     
- Misses         67       70       +3     

Impacted Files	Coverage Δ
src/expression.rs	95.48% <95.65%> (+0.07%)	:arrow_up:
src/term.rs	93.13% <96.55%> (+1.35%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 52d7599...cee2b0f. Read the comment docs.

[shnewto]

Hi @DrunkJon I was thinking about your comment about unexpected behavior

a | Expression::from_parts(vec![b,c]) != Expression::from_parts(vec![a,b,c])

That actually looks right to me, I view a | Expression::from_parts(vec![b,c]) as multiple expressions, i.e. each or'd value is an expression that a rule/lhs can evaluate to.

I'd hope that something like this would be true instead:

`a | Expression::from_parts(vec![b,c]) == vec![ Expression::from_parts(vec![a]),Expression::from_parts(vec![b,c])]`

well, conceptually at least. what's important is that it could be used as the right hand side / rhs of a production composed of the or'd expressions

[shnewto]

@DrunkJon oh 🙃 I wasn't reading the original issue 🤦. I see it suggested something much more like what you've implemented but I'm not sure I'm still on that page, i.e. it working as a union operator. I need a little bit to chew on this.

Thanks for your patience, and thanks again for the PR!

[DrunkJon]

@shnewto I think you're right. The way I currently implemented it doesn't match BNF at all. I was a bit cofused because the original Issue mentions expr = term | term | term but if we wanted to follow BNF it should be prod = expr | expr | expr instead.

I'm going to change the code to use the + operator and look into implementing a BitOr that returns Vec<Expression>.

[shnewto]

Ah that sounds great, thanks @DrunkJon 🙏 🙌 😄

[shnewto]

@DrunkJon looks good, I don't know that I agree with how particular the codecov check is failing 🤔 I think I'll disable it a little later until I get it reporting more meaningful things for a PR.

I did notice after changing from or to add the tests are still named or_operator. Would you mind changing that to add_operator? (plus_operator? addition_operator? hah doesn't really matter to me except that it's not or anymore)

🙏

[DrunkJon]

I don't really get why, but I still get compiler warnings that the mut is not needed in the tests. Is this because there is an immutable version of the operator? I'm not sure if the mutable version of the code actually get's called.

[DrunkJon]

Found the problem, the mutable code was always being called. I updated the tests to also check immutable methods, maybe this will get through the checks?

[DrunkJon]

I forgot to remove debug println!'s sorry! 🙇

[shnewto]

@DrunkJon looks like the codecov checks are happy now 🤩 Thank you!!