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.

Sign up for GitHub

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

Jump to bottom

Support http-api-data #490

Merged
merged 8 commits into from
Oct 12, 2015
Merged

Support http-api-data #490

merged 8 commits into from
Oct 12, 2015

Conversation

fizruk
Copy link
Contributor

@fizruk fizruk commented Oct 9, 2015

This adds ToHttpApiData and FromHttpApiData instances and replaces PathPieces instance bodies with toUrlPiece and parseUrlPieceMaybe.

From user's POV this changes instance for CheckMark to encode in lower-case and parse case-insensitively.

@gregwebs
Copy link
Member

gregwebs commented Oct 9, 2015

Thanks! It just needs a few changes for older versions of GHC

@fizruk
Copy link
Contributor Author

fizruk commented Oct 9, 2015

@gregwebs you have ideas about GHC 7.6 builds? I am not sure what happens there.

@gregwebs
Copy link
Member

gregwebs commented Oct 9, 2015

7.6 was already failing, so don't worry about that. I think cabal is just failing to find an install plan for 7.6 now.
My plan right now is to wait for a 7.6 user to complain that persistent is not installing for them before delving into it.

@fizruk
Copy link
Contributor Author

fizruk commented Oct 9, 2015

Maybe you should place GHC 7.6 builds under allow_failures then?

@gregwebs
Copy link
Member

gregwebs commented Oct 9, 2015

good call. trying that in #491

@fizruk
Copy link
Contributor Author

fizruk commented Oct 9, 2015

Is this PR good for merge?

@gregwebs
Copy link
Member

gregwebs commented Oct 9, 2015

yeah, I will merge and release it after fixing the Travis build. Thanks a lot!
You might want to put an upper bound on http-api-data, but I will trust your judgement there.

@fizruk
Copy link
Contributor Author

fizruk commented Oct 9, 2015

@gregwebs I started to forget about bounds after I've switched to stack, sorry :)

@gregwebs
Copy link
Member

gregwebs commented Oct 9, 2015

I pushed your branch with a merge to master, will merge the branch after that passes: https://travis-ci.org/yesodweb/persistent/builds/84593423

gregwebs
gregwebs reviewed Oct 10, 2015
View reviewed changes
persistent-mongoDB/Database/Persist/MongoDB.hs
import Web.PathPieces (PathPiece (..))
import Web.PathPieces (PathPiece(..))
import Web.HttpApiData (ToHttpApiData(..), FromHttpApiData(..))
import Web.HttpApiData.Internal (parseUrlPieceWithPrefix, readEitherTextData, parseUrlPieceMaybe)
Copy link
Member

Choose a reason for hiding this comment

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

Should this module be Internal if it is used by all the persistent libraries? Internal generally means something that should be avoided if possible and not depended upon by a library. A more usual name would be Utils, but I don't like generic names like that and tend to look for something more like Web.HttpApiData.Parse

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bringing this up! For the first version of http-api-data this was intentional, since I didn't know which helpers I should export and by what name (I do not particularly like current naming).
I have opened fizruk/http-api-data#18 to discuss this.

@fizruk fizruk mentioned this pull request Oct 10, 2015
Closed
@fizruk
Copy link
Contributor Author

fizruk commented Oct 11, 2015

I have updated http-api-data dependency. Now persistent does not use Web.HttpApiData.Internal.

@gregwebs
Copy link
Member

great! I will

  • Add ChangeLog entries
  • bump versions, push tags, and release to hakage

gregwebs added a commit that referenced this pull request Oct 12, 2015
@gregwebs
Merge pull request #490 from fizruk/support-http-api-data
492c7a2 
Support http-api-data
@gregwebs gregwebs merged commit 492c7a2 into yesodweb:master Oct 12, 2015
@fizruk fizruk deleted the support-http-api-data branch October 12, 2015 07:07
@fizruk
Copy link
Contributor Author

fizruk commented Oct 19, 2015

@gregwebs can I do anything to speed this up?

@gregwebs
Copy link
Member

Nope. I created #494 for this

@fizruk
Copy link
Contributor Author

fizruk commented Oct 19, 2015

@gregwebs Great, thanks!

@bitemyapp bitemyapp mentioned this pull request Feb 27, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fizruk @gregwebs