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.

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

Grade school exercise: A student cannot simultaneously be in two grad… #1735

Merged
merged 1 commit into from
Dec 11, 2020
Merged

Grade school exercise: A student cannot simultaneously be in two grad… #1735

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions exercises/grade-school/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@
},
"expected": ["Aimee"]
},
{
"uuid": "dece43c8-3ba5-11eb-8fdf-7f8daeaeb5f2",
Copy link
Member

@ee7 ee7 Dec 24, 2020 •
edited

Choose a reason for hiding this comment

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

Note that this isn't a valid version 4 UUID (because the first character in the third group is 1, not 4).

It looks like all the other UUIDs in problem-specifications are valid version 4 UUIDs.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have the limitation of it being a v4. 😬

Copy link
Member

Choose a reason for hiding this comment

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

I think for v3 we wanted to use v4 I think, but I don't know why anymore 🤷 @iHiD might know

Copy link
Member

Choose a reason for hiding this comment

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

Someone can PR a change for this one. I don't necessarily think there's much value in forcing it to be totally random (instead of also using date-time and MAC address). The purpose is to be unique, and that it still is 🥇

Copy link
Member

@ee7 ee7 Dec 24, 2020 •
edited

Choose a reason for hiding this comment

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

I don't think we have the limitation of it being a v4.

Indeed, it's not currently in the problem-specifications CI, or in configlet lint. But I think it should be. As I understand it, we'll fix the (not-so-unique) uuids that are duplicated between tracks. So I think we might as well enforce a consistent format for uuids on track repos at the same time. Currently, about 2% of uuids on Exercism are not valid version 4 UUIDs that are also lowercase.

I don't necessarily think there's much value in forcing it to be totally random

It's true that there isn't a meaningful difference in the collision probability, but UUID version 4 is the right version for a randomly generated UUID. It's also what configlet uuid generates (in both in the v2 and v3 versions of configlet), and every other problem-specifications uuid is version 4.

Furthermore, a reader who is familiar with RFC 4122 might see a version 1 UUID and assume that the timestamp component is useful, causing them to wonder why the others are version 4.

Someone can PR a change for this one.

I'll do that, but we'd also have to PR to any track that adds the old UUID to a tests.toml file. So far, that's only on the javascript track here. I'll PR there too.

The purpose is to be unique, and that it still is

True. But we shouldn't go too far with that argument - otherwise it implies that we should merge a uuid that looks like this. That is the most unique uuid, though :)

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore, a reader that understands UUIDs might see a version 1 UUID and assume that the timestamp component is useful, causing them to wonder why the others are version 4.

Yeah, that's a good point!

'll do that, but we'd also have to PR to any track that adds the old UUID to a tests.toml file. So far, that's only on the javascript track here. I'll PR there too.

Sweet thanks! I'll ✅ the PR if you ping me, and lets have reviewers ✅ it to, as to reduce the amount of tracks that will need to do this.

True. But we shouldn't go too far with that argument - otherwise it implies that we should merge a uuid that looks like this. That is the most unique uuid, though :)

😏

Copy link
Member

Choose a reason for hiding this comment

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

I'd quite like to have ${PREFIX}-${v4 UUID} everywhere. Where the prefix signifies the context (e.g. this would be PSTC (Problem specs test case) or similar)

"description": "A student can't be in two different grades",
"property": "roster",
"input": {
"students": [
["Aimee", 2],
["Aimee", 1]
],
"desiredGrade": 2
},
"expected": []
},
{
"uuid": "233be705-dd58-4968-889d-fb3c7954c9cc",
"description": "Adding more students adds them to the sorted roster",
Expand Down