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

sync: write tests.toml with reimplements #317

Merged
merged 5 commits into from
May 31, 2021
Merged

Conversation

ErikSchierboom
Copy link
Member

@ErikSchierboom ErikSchierboom commented May 7, 2021

This commits adds a reimplements property to the tests.toml file to help identify re-implemented test cases:

[b9228bb1-465f-4141-b40f-1f99812de5a8]
description = "disallow first strand longer"
reimplements = "919f8ef0-b767-4d1b-8516-6379d07fcb28"

Closes #316

@ee7 This was suggested during one of the weekly meetups, and I thought it made sense. I've create a small PoC. Let me know what you think.

Copy link
Member

@ee7 ee7 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 it. The only downside I see is that a user might think that we actually use the reimplements value in some way. But actually, it's just there to improve readability/understanding, right? Nothing bad happens if the user accidentally edits the reimplements UUID.

I think it's fine, but do you think it's worth trying to explain reimplements in the comment at the top of the file? Now that I look at it again, should we explain the implicit include = true too?

Some nitpicks in #318.

@ErikSchierboom
Copy link
Member Author

I like it. The only downside I see is that a user might think that we actually use the reimplements value in some way. But actually, it's just there to improve readability/understanding, right? Nothing bad happens if the user accidentally edits the reimplements UUID.

That is correct.

I think it's fine, but do you think it's worth trying to explain reimplements in the comment at the top of the file? Now that I look at it again, should we explain the implicit include = true too?

We probably should.

@ErikSchierboom ErikSchierboom marked this pull request as ready for review May 7, 2021 16:43
@ErikSchierboom ErikSchierboom requested a review from ee7 May 7, 2021 16:43
@ErikSchierboom ErikSchierboom force-pushed the output-reimplements-property branch from b5827ec to f2ffb37 Compare May 22, 2021 09:16
ErikSchierboom and others added 4 commits May 28, 2021 14:52
With this commit, `configlet sync` will add a `reimplements` property to
each relevent test case when writing the `tests.toml` file to. This helps
the reader to identify re-implemented test cases. For example:

```toml
[b9228bb1-465f-4141-b40f-1f99812de5a8]
description = "disallow first strand longer"
reimplements = "919f8ef0-b767-4d1b-8516-6379d07fcb28"
```
@ee7 ee7 force-pushed the output-reimplements-property branch from f2ffb37 to 4b8225c Compare May 28, 2021 12:53
@ee7
Copy link
Member

ee7 commented May 28, 2021

Rebased on main - I'll fix the tests in a bit.

Copy link
Member

@ee7 ee7 left a comment

Choose a reason for hiding this comment

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

I'd like to tweak the comments at the top of the tests.toml file, but I didn't find a wording I was happy with. So let's just handle that later - I'll make the PR.

Some potential changes:

  • It looks like "property" isn't actually the preferred terminology for TOML (oops) - I think it's just "key/value pair".
  • We might like to mention configlet sync somewhere, rather than just "regenerating" (which might be confused with configlet generate).

I have another PR that should use this PR as a base (several PRs require changing the integration tests for sync) so I'll merge this.

@ee7 ee7 merged commit b087357 into main May 31, 2021
@ee7 ee7 deleted the output-reimplements-property branch May 31, 2021 06:33
@ee7 ee7 changed the title sync: output reimplements property sync: write tests.toml with reimplements May 31, 2021
@ErikSchierboom
Copy link
Member Author

Thanks!

It looks like "property" isn't actually the preferred terminology for TOML (oops) - I think it's just "key/value pair".

Whoops.

We might like to mention configlet sync somewhere, rather than just "regenerating" (which might be confused with configlet generate).

Yeah I considered this. I struggled a bit with the wording, so I omitted it for now.

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.

sync: Add reimplements property to tests.toml file
2 participants