Skip to content

Remove unneeded nesting #1798

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
May 6, 2021
Merged

Remove unneeded nesting #1798

merged 18 commits into from
May 6, 2021

Conversation

ErikSchierboom
Copy link
Member

@ErikSchierboom ErikSchierboom commented May 2, 2021 •
edited
Loading

The canonical data for isogram and prime-factors contain nesting, but as there is only one level of nesting, this does not add any value.

This can break test generators, but as everything is still in flux anyway, I think this is a good time to do this.

Note: please don't squash merge to retain the individual commits

@ErikSchierboom ErikSchierboom force-pushed the remove-unneeded-nesting branch from 7c61def to df51e30 Compare May 2, 2021 07:29
@ErikSchierboom ErikSchierboom force-pushed the remove-unneeded-nesting branch from df51e30 to 36e3acf Compare May 2, 2021 07:30
@ErikSchierboom ErikSchierboom changed the title isogram: remove unneeded nesting remove unneeded nesting May 2, 2021
@ErikSchierboom ErikSchierboom changed the title remove unneeded nesting Remove unneeded nesting May 2, 2021
@ErikSchierboom ErikSchierboom mentioned this pull request May 2, 2021
Merged
SleeplessByte
SleeplessByte approved these changes May 2, 2021
View reviewed changes
Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

Yeah, I mentioned this when updating JS to spec. Agreed that THIS is the best time to do this.

@ee7
Copy link
Member

ee7 commented May 5, 2021

I think there are many more exercises for which we'd have to make the same change. Here is an incomplete list:

@ErikSchierboom
Copy link
Member Author

I'd like to get all those fixed in this PR too. I'll take a little time to find all of them.

@ee7
Copy link
Member

ee7 commented May 5, 2021

See previous discussion: #1671

@ErikSchierboom
Copy link
Member Author

I think the following bash script finds all these unneeded nesting exercises:

find . -name canonical-data.json -exec jq '. | select(.cases | length == 1) | select(.cases[] | .cases != null) | .exercise' {} ; | uniq | sort

This outputs:

"acronym"
"alphametics"
"book-store"
"etl"
"food-chain"
"gigasecond"
"house"
"micro-blog"
"nucleotide-count"
"ocr-numbers"
"pascals-triangle"
"phone-number"
"protein-translation"
"rotational-cipher"
"secret-handshake"
"trinary"

@ErikSchierboom ErikSchierboom force-pushed the remove-unneeded-nesting branch from e5f44af to f393a65 Compare May 5, 2021 12:58
@ErikSchierboom ErikSchierboom force-pushed the remove-unneeded-nesting branch from f393a65 to 97fd158 Compare May 5, 2021 13:00
@ErikSchierboom
Copy link
Member Author

I found 18 exercises for which there was unneeded nesting, which I've removed in this PR. Each exercise has its own commit to make reviewing easier.

@SaschaMann SaschaMann merged commit 5fc501b into main May 6, 2021
@SaschaMann SaschaMann deleted the remove-unneeded-nesting branch May 6, 2021 05:32
@ee7
Copy link
Member

ee7 commented May 7, 2021

As far as I know, this PR flattened 2 levels of cases to 1, where possible.

Are there any canonical-data.json files with 3 levels of cases that could be 2?

This PR probably broke most generators for these exercises. Do we have a list of tracks that use generators so that we can let some maintainers know?

@ErikSchierboom
Copy link
Member Author

Are there any canonical-data.json files with 3 levels of cases that could be 2?

No idea. It's less likely, but I'll check next week.

Do we have a list of tracks that use generators so that we can let some maintainers know?

Nope, unfortunately we don't.

@BethanyG
Copy link
Member

BethanyG commented May 9, 2021

Just discovered that it broke Python. Took me a while to figure out what was going on, but glad I found this issue. Working on it now. 😆

@BethanyG BethanyG mentioned this pull request May 10, 2021
Merged
@ErikSchierboom
Copy link
Member Author

Sorry about that @BethanyG! It will have broken more tracks, but we made this change to fix this slightly annoying issue now, when all tracks are in flux anyway.

ee7 added a commit to exercism/configlet that referenced this pull request May 31, 2021
@ee7
tests(binary): update problem-specifications hash (#342)
38e1bc1 
This commit updates our tests of the configlet binary to use the latest
`problem-specifications` commit. We do this because we want the update
that removes unnecessary nesting [1] before we make configlet write each
tests.toml `description` in a longer form that shows nesting (see #301).

[1] exercism/problem-specifications#1798
ee7 added a commit to exercism/configlet that referenced this pull request Jun 2, 2021
@ErikSchierboom @ee7
sync: write fuller description for nested test cases (#301)
aa23b4d 
Some exercises in the `problem-specifications` repo have a
`canonical-data.json` file that uses nesting, meaning that there is more
than one `cases` property.

`configlet` handled these nicely, but each `description` in each
`tests.toml` file contained only the description of the test case
itself. This could be unclear, and was especially confusing when test
cases in different nested objects had the same description. Each
`description` is purely for humans to read - it isn't used otherwise.

With this commit, `configlet sync` writes the "full description" to the
`tests.toml` file. For example, a `tests.toml` for the `triangle` 
exercise might previously contain (omitting other test cases for
clarity):

```toml
[3022f537-b8e5-4cc1-8f12-fd775827a00c]
description = "sides may be floats"

[adb4ee20-532f-43dc-8d31-e9271b7ef2bc]
description = "sides may be floats"

[26d9d59d-f8f1-40d3-ad58-ae4d54123d7d]
description = "sides may be floats"
```

But with this commit:

```toml
[3022f537-b8e5-4cc1-8f12-fd775827a00c]
description = "equilateral triangle → sides may be floats"

[adb4ee20-532f-43dc-8d31-e9271b7ef2bc]
description = "isosceles triangle → sides may be floats"

[26d9d59d-f8f1-40d3-ad58-ae4d54123d7d]
description = "scalene triangle → sides may be floats"
```

For the canonical data for this exercise, see:
https://github.com/exercism/problem-specifications/blob/f17f457fdc06/exercises/triangle/canonical-data.json

We decided that putting everything in `description` is better than:
- adding extra key/value pair(s) named e.g. `category` or `super`
- adding TOML comments
- using sub-tables

While working on this PR, we noticed that 18 exercises in
`problem-specifications` had unnecessary nesting. We simplified them in
exercism/problem-specifications#1798

Closes: #202

Co-authored-by: ee7 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolf99 wolf99 wolf99 approved these changes

@SleeplessByte SleeplessByte SleeplessByte approved these changes

@SaschaMann SaschaMann Awaiting requested review from SaschaMann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ErikSchierboom @ee7 @BethanyG @wolf99 @SleeplessByte @SaschaMann