Implemented comparison operators (min and max) #37
Conversation
ilijapuaca
force-pushed
the
feature/comparison
branch
from
6040e96
to
df6eb10
Compare
|
I'm used to having about 120 as a reasonable size and I think 90 makes it hard to write functions that don't get very contorted, but @broadwaylamb prefers 90 so that you can fit 2 side by side on a 13" screen. FWIW, I would prefer a higher character limit. |
|
Thank you so much for taking this! I’m going to look through this soon. |
|
Any chance we can add this PR to #1 and the Project, I'd do it myself but I'm not able to. Just trying to make sure no duplicate work gets done as PRs might take a while to merge |
|
Can you please synchronize this with the master branch? |
ilijapuaca
force-pushed
the
feature/comparison
branch
from
df6eb10
to
b651b5a
Compare
|
Done. Did you get a chance to take a look at the issue with the test I mentioned above? I was busy this last week, hoping to get some more work done this weekend. |
|
Sounds great! I'll address any feedback as soon as you get the review done |
broadwaylamb
added
the
missing functionality
|
I'm really sorry it's been so long. I've done some refactoring that should make implementing this kind of operators trivial, and I'm currently working on it in #76. I'll definitely use the test cases that you wrote, so thank you @ilijapuaca for your time. |
I implemented most of this a few days back, mostly reusing what's currently in the repo.
There is a bug with max operator with custom comparison closure, hence the test fail, but I thought I'd open up a PR and see if you guys have any ideas
The bug itself is probably pretty straight-forward to fix, I'm just having a hard time reasoning what min and max operators with custom comparison closures are supposed to do in the first place. I assume it's just a matter of somehow "inverting" the logic for min and max operators, after the custom closure is executed. The test that's failing is called
testCustomMaxSendsCorrectValue.Let me know how all of this looks, and I can address any feedback asap.
On a side note -- one thing that slowed me down quite a bit was trying to squeeze in some of the code with lengthy generic names into single lines. Is there a reason with went for 90 char/line limit? It seems pretty strict, leaving basically half of my laptop screen empty. On projects I previously worked on, the number was usually in 130-150 range. Just curious if it's open for debate or not :)