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

Improve documentation around migration safety #1277

Merged
merged 5 commits into from
Jun 23, 2021

Conversation

ivanbakel
Copy link
Contributor

@ivanbakel ivanbakel commented May 26, 2021

The Haddocks around CautiousMigration were ambiguous, since it wasn't clear if the flag signified whether each migration was safe or not - and there is at least one instance in the code that gets it wrong.

This corrects that error, opting to make the addMigration documentation match the code's behaviour.

Personally, I think the better approach to this mess would be to replace all the use of (Bool, Text) with a datatype that enforces better usage through a stricter API - something like

data CautiousSql = MkCautiousSql { ... } -- Hide this constructor

-- Expose these smart constructors
mkSafeSql :: Sql -> CautiousSql
mkSafeSql = ...

mkUnsafeSql :: Sql -> CautiousSql
mkUnsafeSql = ...

-- Expose these instead of record fields
isUnsafe :: CautiousSql -> Bool
isUnsafe = ...

rawSql :: CautiousSql -> Sql
rawSql = ...

-- Maybe even a smart destructor?
ifSafe :: (Bool -> Sql -> a) -> CautiousSql -> a
ifSafe = ...

That way consumers could not be misled about the nature of the Bool value. But I thought that would be quite drastic and breaking, so I didn't want to make it my first suggestion.

Fixes #1081.

Before submitting your PR, check that you've:

  • Documented new APIs with Haddock markup
  • Added @since declarations to the Haddock
  • Ran stylish-haskell on any changed files.
  • Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@ivanbakel
Copy link
Contributor Author

I've discovered this fixes an issue, so I'll add that into the PR message.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Let's make this a documentation change instead of a behavior change. That way we can release it as a patch fix. If we're changing behavior, that'd need to be in a major version bump.

@@ -200,7 +200,7 @@ addMigration
-> Sql
-- ^ A 'Text' value representing the command to run on the database.
-> Migration
addMigration isSafe sql = lift (tell [(isSafe, sql)])
addMigration isSafe sql = lift (tell [(not isSafe, sql)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be a breaking change in behavior. Let's instead change the documentation? That way, we won't break any existing code, and folks writing new code will be able to tell from the docs how to do it right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done now.

persistent/Database/Persist/Sql/Types.hs Outdated Show resolved Hide resolved
ivanbakel and others added 4 commits June 23, 2021 16:37
The Haddocks around CautiousMigration were ambiguous, since it wasn't
clear if the flag signified whether each migration was safe or not - and
there is at least one instance in the code that gets it wrong.

This corrects that error, opting to make the `addMigration`
documentation match the actual behaviour of the code.
Co-authored-by: Matt Parsons <parsonsmatt@gmail.com>
@parsonsmatt
Copy link
Collaborator

looks great! persistent-2.13.0.3 was released since, but I'll get this out as persistent-2.13.0.4.

@parsonsmatt parsonsmatt merged commit 8cfab40 into yesodweb:master Jun 23, 2021
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.

The documentation for addMigration is confusing
2 participants