Skip to content

Order of canonical-data keys #1705

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

Closed
4 tasks
SaschaMann opened this issue Oct 13, 2020 · 9 comments · Fixed by #1960
Closed
4 tasks

Order of canonical-data keys #1705

SaschaMann opened this issue Oct 13, 2020 · 9 comments · Fixed by #1960

Comments

@SaschaMann
Copy link
Contributor

SaschaMann commented Oct 13, 2020

From @ee7 in #1703 (review)

But two questions:

  1. Do we have an official ordering that the keys are supposed to be in?

  2. If so, should we enforce the key order during CI? Maybe it helps with readability when adding tests. The ordering seemed pretty consistent before, but there will be nearly double the number of keys now.

I notice that the reimplements in this PR is after uuid, but when I made my recent PRs I instead ordered the keys as I saw them in #1674, that is:

  {
    "uuid": "82d32c2e-07b5-42d9-9b1c-19af72bae860",
    "description": "two times one is two",
    "comments": ["Expected value is changed to 2"],
    "reimplements": "e46c542b-31fc-4506-bcae-6b62b3268537",
    "property": "twice",
    "input": {
      "number": 1
    },
    "expected": 2
  }

Tasks:

  • Decide if there should be an order
  • Decide on an order
  • Convert all data to the right order
  • Add a CI check for it
@ErikSchierboom
Copy link
Member

I think the order for virtually all of the existing canonical data is:

  1. "uuid"
  2. "description"
  3. "comments"
  4. "property"
  5. "input"
  6. "expected"

I'm fine with keeping this as is. Maybe we should add the "reimplements" key directly after the "uuid" key?

  1. "uuid"
  2. "reimplements"
  3. "description"
  4. "comments"
  5. "property"
  6. "input"
  7. "expected"

@SaschaMann
Copy link
Contributor Author

Maybe we should add the "reimplements" key directly after the "uuid" key?

I'd like that because it keeps the unreadable part away from the rest

@ee7
Copy link
Member

ee7 commented Oct 14, 2020

@ErikSchierboom And "scenarios"? In #1674 it was written last in:

{
  "uuid": "25a8463d-560f-4a42-9be7-b79d00cd28a4",
  "description": "64",
  "property": "square",
  "input": {
    "square": 64
  },
  "expected": 9223372036854775808,
  "scenarios": ["big-integers"]
}

@ErikSchierboom
Copy link
Member

Ah, yes, forgot about that 🤦 Scenarios is basically metadata, so it would perhaps make sense to group it with the other metadata. Maybe:

  1. "uuid"
  2. "reimplements"
  3. "scenarios"
  4. "description"
  5. "comments"
  6. "property"
  7. "input"
  8. "expected"

or

  1. "uuid"
  2. "reimplements"
  3. "description"
  4. "comments"
  5. "scenarios"
  6. "property"
  7. "input"
  8. "expected"

I think I have a slight preference for the latter.

@iHiD
Copy link
Member

iHiD commented Oct 14, 2020

I prefer later but not a hard preference.

@SleeplessByte
Copy link
Member

I prefer the latter too.

@ErikSchierboom
Copy link
Member

Okay, lets go the following then:

  1. "uuid"
  2. "reimplements"
  3. "description"
  4. "comments"
  5. "scenarios"
  6. "property"
  7. "input"
  8. "expected"

@cmccandless
Copy link
Contributor

Wouldn't this be a change to configlet fmt?

@ErikSchierboom
Copy link
Member

It would, but we're right in the middle of migrating configlet which could take a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants