Skip to content

Update yesod-auth and yesod-persistent to persistent-2.9#1516

Merged
snoyberg merged 3 commits intoyesodweb:masterfrom
iand675:master
Oct 14, 2018
Merged

Update yesod-auth and yesod-persistent to persistent-2.9#1516
snoyberg merged 3 commits intoyesodweb:masterfrom
iand675:master

Conversation

@iand675
Copy link
Copy Markdown
Contributor

@iand675 iand675 commented Jun 4, 2018

This is an accompanying PR to fix changes from yesodweb/persistent#812

Could use advice on the following, not sure whether to bump the patch version or the minor version.

  • Bumped the version number

@snoyberg
Copy link
Copy Markdown
Member

snoyberg commented Jun 4, 2018

It would be better to use CPP to support both the old and new version of persistent. And I'd recommend a minor version bump.

@@ -1,5 +1,5 @@
name: yesod-auth
version: 1.6.3
version: 1.7.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to PVP rules, this is a major version bump. A minor version bump would be 1.6.4.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. I was thinking about it in terms of semantic versioning. That explains my confusion.

build-depends: base >= 4 && < 5
, yesod-core >= 1.6 && < 1.7
, yesod-persistent >= 1.6 && < 1.7
, yesod-persistent >= 1.7 && < 1.8
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason that the version bump is needed for yesod-form?

@@ -1,5 +1,5 @@
name: yesod-persistent
version: 1.6.0
version: 1.7.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should probably be 1.6.1.

@iand675 iand675 force-pushed the master branch 2 times, most recently from 37e9c9e to 4641fa0 Compare June 7, 2018 09:07
@iand675
Copy link
Copy Markdown
Contributor Author

iand675 commented Jun 8, 2018

@snoyberg I believe I've addressed your feedback.

@iand675
Copy link
Copy Markdown
Contributor Author

iand675 commented Jun 8, 2018

Although obviously the persistent one needs merging and releasing before this one has value.

@snoyberg
Copy link
Copy Markdown
Member

Thanks. Sorry, just one more request: could you add a ChangeLog entry to each package that has changed explaining the support for persistent 2.9?

@iand675
Copy link
Copy Markdown
Contributor Author

iand675 commented Jun 13, 2018

@snoyberg done. Thanks as always for your hard work!

Copy link
Copy Markdown
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

LGTM. Should I wait to merge when persistent 2.9 is released?

@iand675
Copy link
Copy Markdown
Contributor Author

iand675 commented Jun 13, 2018

I don't think it's strictly necessary to wait, but I don't think the persistent PR has been reviewed at all yet. So... if there are any requested interface changes there it might be problematic.

@snoyberg
Copy link
Copy Markdown
Member

In that case, I'd say let's hold off for now. Would you mind pinging this issue when the 2.9 release happens?

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.

2 participants