Skip to content
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

Complex numbers: Sync tests with problem-specifications #567

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

tasxatzial
Copy link
Contributor

@tasxatzial tasxatzial commented Jul 11, 2023

Partially addresses #520

Tests are now in sync, missing tests have been implemented, order of the actual and expected results has been fixed.

I'll try to address the issue with the floats in a subsequent PR since this one is changes a lot of things.

This PR is not expected to break existing solutions

@github-actions
Copy link
Contributor

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jul 11, 2023
@bobbicodes
Copy link
Member

For some reason it's not giving me the option to reopen this... weird

@bobbicodes
Copy link
Member

Oh, now it is :)

@bobbicodes
Copy link
Member

I still find it odd that the expected/actual results depend on the order in the = expression. I'm trying to see if that is documented anywhere but it doesn't seem to be.

@tasxatzial
Copy link
Contributor Author

Don't do anything yet with this PR. I need to check few things.

@tasxatzial
Copy link
Contributor Author

tasxatzial commented Jul 11, 2023

Ok, so i had a suspicion that the order of actual expected is nothing more than a convention. The Clojure api here shows an example with expected being the initial form and actual being the computed result. Re-writing (= 5 (+ 2 2)) as (= (+ 2 2) 5) simply switches the initial form, but this doesn't really make any difference because the actual form remains the same. I've tested this with the online editor and with lein and both don't care about the order because the initial form isn't evaluated.

The confusion about the convention arises when we realize that not every test tool behaves like this. Cursive, for example, reports something completely different.

(is (= 5 (+ 2 2))) -> Expected 4, Actual 5
(is (= (+ 2 2) 5)) -> Expected 5, Actual 4

So not only it evaluates the initial form, but now the order makes a difference. It treats the first argument of is as the actual and the second as the expected. But apparently for most test tools which behave similarly, the order comes from the Clojure API and is the exact opposite: First is expected, second is actual. Of course, there's no convention dictated by the API, but in all their examples show that the value we are checking against goes first and that's what they've chosen to call expected. So, for tools that evaluate the forms, the order does matter.

How do i know this? I just asked the Cursive author here. So I'll leave the order as it is in this PR and hopefully some day he fixes Cursive.

@tasxatzial
Copy link
Contributor Author

This one is finished. Tested with random community solutions and i've found one that fails due to floating point inaccuracies. The good news is that the next PR, which is ready, will resolve those issues.

Copy link
Member

@bobbicodes bobbicodes left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@bobbicodes bobbicodes merged commit 2d1fba2 into exercism:main Jul 12, 2023
2 checks passed
@tasxatzial tasxatzial deleted the complex-numbers-p1 branch September 22, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants