Skip to content

triangle: actively ignore cases with float input #338

Merged
sshine merged 5 commits intoexercism:masterfrom
marionebl:resync-triangle
Jun 27, 2019
Merged

triangle: actively ignore cases with float input #338
sshine merged 5 commits intoexercism:masterfrom
marionebl:resync-triangle

Conversation

@marionebl
Copy link
Copy Markdown
Contributor

No description provided.

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Jun 26, 2019

I don't think that this is a good idea.

Here we perform structural equality on floats.

I think the exercise is complicated significantly if we are to introduce rounding-error testing.

I would also say that this is a bug in OCaml. See Why can't I compare reals in Standard ML?.

@marionebl
Copy link
Copy Markdown
Contributor Author

I don't think that this is a good idea.

Here we perform structural equality on floats.

I think the exercise is complicated significantly if we are to introduce rounding-error testing.

I would also say that this is a bug in OCaml. See Why can't I compare reals in Standard ML?.

The test dataset contains floating point numbers though. Should we filter out those test cases then?

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Jun 26, 2019

The test dataset contains floating point numbers though. Should we filter out those test cases then?

Yes. triangle's canonical data gives this comment:

      "comments": [
        " Your track may choose to skip this test    ",
        " and deal only with integers if appropriate "
      ],

When exercism/problem-specifications#1518 merges, we can push a change to triangle's canonical tests that changes these comments into an optional: "floating-point" and filter based on this in the test generator.

@marionebl
Copy link
Copy Markdown
Contributor Author

Ok, so filtering cases was more involved than I would have guessed. Anyway - adapted the test-generator as requested.

@marionebl marionebl changed the title triangle: accept float sides triangle: actively ignore cases with float input Jun 26, 2019
Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Excellent work.

I went and pushed for the merge of the "optional": "..." feature in canonical-data.json to make this easier, but it seems you found a way with Yojson to filter out the non-integer test cases. Nice job.

@sshine sshine merged commit 58e3989 into exercism:master Jun 27, 2019
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