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

Fix ToSchema Version instance. #2978

Merged
merged 7 commits into from
Jan 16, 2023
Merged

Fix ToSchema Version instance. #2978

merged 7 commits into from
Jan 16, 2023

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Jan 12, 2023

This partially reverts commit 6f91151 (#2965).

I think I've overkilled it, but I wanted it to be safe!

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

This partially reverts commit 6f91151.
@fisx fisx temporarily deployed to cachix January 12, 2023 11:51 — with GitHub Actions Inactive
@fisx fisx temporarily deployed to cachix January 12, 2023 11:51 — with GitHub Actions Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 12, 2023
@fisx fisx temporarily deployed to cachix January 12, 2023 11:56 — with GitHub Actions Inactive
@fisx fisx temporarily deployed to cachix January 12, 2023 11:56 — with GitHub Actions Inactive
@fisx fisx marked this pull request as ready for review January 12, 2023 11:57
constructed =
-- (fromEnum doesn't work as soon as we drop support for V0.)
(\v -> (read . Imports.tail . show $ v, v)) <$> [minBound @Version ..]
explicit =
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like how easy it is to forget or make a mistake about this, but I don't have a better idea in mind at the moment. 😰

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean? how would you do a mistake here that'd go uncaught by the CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean forgetting to add / update versions and it not being caught by GHC/ type checking. But hopefully CI is enough to prevent us from messing up 🤞

@fisx fisx requested a review from elland January 12, 2023 12:18
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

Can't we just revert and use an explicit list of versions? This is a mess.

@fisx
Copy link
Contributor Author

fisx commented Jan 13, 2023

Can't we just revert and use an explicit list of versions? This is a mess.

my idea was that this way, you get an error in the integration tests if you forget to update ToSchema. and i don't think it's that hard to read.

but i'll let you decide: please approve or reject.

Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

If you forget to add a new version that has some changes, you'll get an error in the tests anyway as soon as you make use of the new stuff. On that basis, I'm for restoring the simple code.

@fisx fisx requested a review from pcapriotti January 16, 2023 08:48
@fisx fisx temporarily deployed to cachix January 16, 2023 08:52 — with GitHub Actions Inactive
@fisx fisx temporarily deployed to cachix January 16, 2023 08:52 — with GitHub Actions Inactive
@fisx fisx merged commit 8816b3f into develop Jan 16, 2023
@fisx fisx deleted the fixup-20230112 branch January 16, 2023 11:17
@fisx fisx mentioned this pull request Feb 13, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants