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

Triangle - A bug or an extension? #289

Merged
merged 12 commits into from
Feb 14, 2024
Merged

Triangle - A bug or an extension? #289

merged 12 commits into from
Feb 14, 2024

Conversation

cpmachado
Copy link
Contributor

Hello there,

During a code review with bomber34, as I'm quite new to Prolog, he pointed out the following(from my solution):

?- triangle(1.0, 1, 1).
false

This happens, because my solution which was similar to the example enforces type equality, and it shouldn't or should it?
I took the liberty to submit this patch, in case it's to be considered a bug.

Best regards,
cpmachado

Copy link

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


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 Jan 25, 2024
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I like this change, as it forces students to think about equality in Prolog.

My only suggestion would be is to add something to the docs to make students aware of this. The best way to do that is to add a .docs/instructions.append.md to the exercise, which could then make students aware (it'll get shown automatically in the editor and added to the instructions when downloaded via the CLI). See https://github.com/exercism/elixir/blob/main/exercises/practice/nth-prime/.docs/instructions.append.md for an example.

@cpmachado
Copy link
Contributor Author

@ErikSchierboom My concern which wasn't maybe properly addressed, is that as my branch is, it enforces solutions without type equality. The implications are that many solutions that are currently working would be failing.
Should this change in tests be made as optional? as an extra task? bonus cookie, you know?

Going to address the requested change in the instructions append.

Looking forward for your reply :-)

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I'm looking into the test runner to see what happens when we mark a test as a bonus test, but unfortunately it does run them: exercism/prolog-test-runner#37

We run the tests with this command: https://github.com/exercism/prolog-test-runner/blob/main/bin/run.sh#L39:

swipl -f "${implementation_file}" -s "${tests_file}" -g run_tests,halt -t 'halt(1)' -- --all

Is there a way to ignore bonus tests?

@cpmachado
Copy link
Contributor Author

I'll look into it later in the day, when I have time,

@cpmachado
Copy link
Contributor Author

cpmachado commented Feb 1, 2024

@ErikSchierboom Just fixed the tests and added the bonus flag as pending was before in 30debbe

To run bonus tests

swipl -f "${implementation_file}" -s "${tests_file}" -g run_tests,halt -t 'halt(1)' -- --bonus

exercises/practice/triangle/triangle_tests.plt Outdated Show resolved Hide resolved
Comment on lines 65 to 66
test(sides_are_a_mix, condition(bonus)) :-
triangle(5, 4, 5.0, "isosceles").
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of having the bonus tests be in a separate section, together at the end of the file. That way the student can slowly work through the required tests without getting involved in the bonus tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good afternoon(GMT),
Yesterday in the evening I was checking the docs better, and as such we can improve this kind of approach with begin_tests/2.
I've done as you requested.
I did change the indentation, as the previous was getting on my nerves, I can revert that later.

I was just considering if we should just have pending condition, as I'm doing in bonus, for suites, instead of individual tests.

exercises/practice/triangle/.docs/instructions.append.md Outdated Show resolved Hide resolved
exercises/practice/triangle/.docs/instructions.append.md Outdated Show resolved Hide resolved
@cpmachado cpmachado mentioned this pull request Feb 2, 2024
11 tasks
@cpmachado
Copy link
Contributor Author

just linking this issue, apparently it is the last task #81

@cpmachado
Copy link
Contributor Author

@ErikSchierboom Any more requests?

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for being patient with me.

@ErikSchierboom ErikSchierboom merged commit 4df307f into exercism:main Feb 14, 2024
3 checks passed
@cpmachado cpmachado deleted the triangle-extension branch February 14, 2024 14:58
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.

2 participants