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

Remove the Default instance for ExtrasOptions. #23

Merged
merged 2 commits into from
May 22, 2019

Conversation

23Skidoo
Copy link
Contributor

No description provided.

module Database.PostgreSQL.PQTypes.ExtrasOptions
( ExtrasOptions
, eoForceCommit
, eoEnforcePKs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why export fields separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that clients would be forced to use

defaultExtrasOptions { eoForceCommit = ...
                     , ... 
                     }

to construct ExtrasOptions values instead of

ExtrasOptions { eoForceCommit = ...
              , ... 
              }

which will break if we add additional ExtrasOptions fields. Also lets us add new fields to ExtrasOptions without bumping the major version.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

That seems to be a common pattern, for example with aeson's Options and defaultOptions:
https://hackage.haskell.org/package/aeson-1.4.3.0/docs/Data-Aeson.html#t:Options

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if someone wants it to break as they are interested in knowing any new options that might be added in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

@arybczak that's a good point, because adding options without bumping major version might actually be a bad thing. It would constrain us to, and force us to be careful that the default for the newly added field does not affect the behaviour, as otherwise we might be silently breaking clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although there's a counterargument to that: if the change is significant, say hypothetically eoForceCommit is split into two separate options, then eoForceCommit :: Bool -> ExtrasOptions ceases to exist and becomes a major version bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally users don't, but I can add an .Internal module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not complicate things. Exporting the constructor is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

@arybczak
Copy link
Collaborator

Apart from the export nitpick it looks good 👍

@23Skidoo 23Skidoo force-pushed the remove-default-instances branch 2 times, most recently from 7e1bbe8 to d4a18fd Compare May 22, 2019 12:11
@23Skidoo 23Skidoo merged commit aba2480 into master May 22, 2019
@23Skidoo 23Skidoo deleted the remove-default-instances branch May 22, 2019 13:42
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.

None yet

3 participants