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

Add RateOrder types #2

Merged
merged 25 commits into from Sep 14, 2016

Conversation

Projects
None yet
2 participants
@alexeyzab
Owner

alexeyzab commented Aug 24, 2016

Hi there!

Making this pull request early because I have a bunch of questions about how to do certain things.

I've been looking at bazaari and how the types are implemented there and was wondering if I should add the "un-" accessors to the dataypes, like here:

newtype PostalCode = PostalCode { unPostalCode :: T.Text } deriving (Eq, Show)

Beyond that I am also not sure if I should add similar functions to PostalCode, AddressLine and so on:

makeAddressLine :: T.Text -> Either T.Text AddressLine
makeAddressLine line
| T.length line == 0 = Left "Empty line"
| T.length line > 180 = Left "line is too long"
| T.length line <= 180 = Right $ AddressLine line

I couldn't find any specific information on what the limits are for those fields for Shipwire. Are those supposed to be arbitrary, "common sense" values or is it actually stated somewhere?

As you can see in the defaultShipTo I use T.pack everywhere to convert Strings to Text.

defaultShipTo =
  ShipTo (AddressLine (T.pack "15 Bergen ave")) (AddressLine (T.pack ""))
         (AddressLine (T.pack "")) (City (T.pack "Jersey City"))
         (PostalCode (T.pack "123456")) (Region (T.pack "US")) (Country (T.pack "US"))
         False False

Is that normal? Feels dirty.

Another thing that confuses me a bit is the API restrictions on what is required and what isn't.
For example, if we look at the model schema of the Request class, we'll see this:
"required": [ "country" ],
Does this mean that all the other things inside shipTo are actually not required?
A bit below that I also found another required clause:
"required": [ "shipTo", "items" ],
It's nested one level above the country one. If shipTo is required here, does that mean all the nested fields in shipTo are required as well? Doesn't seem to be that way, otherwise what's the point of specifying the country separately? I might be wrong, of course.

Thank you!

UPD: Ignore the question about the accessors, just realized that the SKU datatype you've made had one. I'll add them to all the other ones in a later commit. My bad.

UPD2: TIR (Today I Remembered) that OverLoadedStrings allows you to write Text without the need of T.pack everywhere. That covers another question.

P.S. One of my vim plugins aligns the imports and things like that which adds additional changes in git. If that's too annoying, I will turn it off.

alexeyzab added some commits Aug 24, 2016

Add RateOrder types
This implements the additional types for RateOrder.

There's more to be done here, but this is an initial commit for these
changes since I am not entirely sure how to proceed with several
different things.
Add Items datatype
This is the final dataype for the Request class for the Rate endpoint.
Need to do more editing here, maybe sort the declarations so it's a bit
more readable.
@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Aug 25, 2016

If that's too annoying, I will turn it off.

That's fine, do as thou wilt.

I'd skip fussing over smart constructors too much for now. More of a capstone than a cornerstone. Focus on making something that can work and covers functionality, then clean up the "correct by construction" story.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Aug 25, 2016

Got it, will do.

Thanks!

UPD: Issuing POST to the /rate endpoint now works. Added some scaffolding to the main function in Client.hs to test it. You can also check what we are sending by doing BSL.pustStrLn $ encode defaultRate.

alexeyzab added some commits Aug 25, 2016

Change Items to a type synonym for [ItemInfo]
Realized that it should be a list of SKUs and quantity values.
Finish the Rate Request datatypes
Sending the defaultRate returns the 200 status now.
Add functions to test the /rate endpoint
This is a placeholder for now, used to check if the types I've been
building work.
Add back Generic instances
Figured out how to get rid of the record syntax by using the
`unwrapUnaryRecords`.
Implement initial FromJSON instances
This is still WIP, there are a lot more instances that need to be
implemented for this endpoint. I wanted to make sure I am on the right
track here.

The client's main function now sends out an encoded Haskell datatype
and decodes the response.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Aug 29, 2016

@bitemyapp I've added some initial FromJSON instances, if you could take a look at those, that would be much appreciated. There is still plenty to do, just want to make sure I am on the right track.

Also, I know certain datatypes need to be stricter than just Text, will work on that once I get this endpoint working.

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Aug 29, 2016

@alexeyzab looks fine so far to me. Let me know if you have questions :)

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Aug 29, 2016

@bitemyapp I have a question about the overall design of this library.

From what I understand, after we write the ToJSON and FromJSON instances, we start writing functions inside the Client module that would actually talk to the endpoints and accept arguments in the form of Haskell datatypes and header parameters.

Is that overview correct or am I misunderstanding something?

Thank you!

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Aug 30, 2016

@alexeyzab seems accurate to me, one thing I'd note is that the client interface in Bloodhound is pretty spare, however, we try to avoid users ever having to touch the URIs themselves beyond providing a host and in the case of Shipwire, it's just beta vs. prod.

Example: https://github.com/bitemyapp/bloodhound/blob/master/src/Database/Bloodhound/Client.hs#L510-L515

note: joinPath

Don't go out of your way to mimic Bloodhound literally, I'm just pointing it out as a guidepost. Keep it simple at first, whatever naturally makes sense to you as you proceed. Refinements can happen later.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Aug 30, 2016

@bitemyapp got it, thanks!

alexeyzab added some commits Sep 3, 2016

Finish adding types for Response class
This should cover all of the JSON we get in response from this endpoint.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 3, 2016

@bitemyapp Hey Chris! I've finished the datatypes for this endpoint's response. I have a couple of questions.

I am using Floats in a few places, this is probably not right. When the result is 0 it gets displayed as 0.0. And there is also a problem with rounding numbers, Float omits a few digits. What would be a better datatype here? Should it be solved with Float and a smart constructor instead?

I am also not sure about the record syntax I've been using, specifically prefixing certain types and not prefixing others. For example:

data PieceLength =
  PieceLength {
   amount :: Integer
 , units  :: Text
 } deriving (Eq, Generic, Show)

vs.

data Carrier =
  Carrier {
    carrierCode       :: Text
  , carrierName       :: Text
  , carrierProperties :: [Text]
  } deriving (Eq, Generic, Show)

Thank you!

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 3, 2016

@alexeyzab usually we'll use Double instead of Float for the extra precision, but I think you're asking in regard to the costs/currencies. You never want to use a floating point type for that, instead you want fixed precision. I think you want E2 from Data.Fixed.

re: prefixing - just prefix everything consistently. We can slap classy lenses on it if the prefixes start irritating you.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 3, 2016

@bitemyapp Gotcha, will do. What I had in mind was subweights stuff like: "amount": 0.83628230902778. Float cuts that off too soon, I think Double should be fine here.

Alright, going to do that then. Thank you!

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 3, 2016

Oh oh, weights. Right. Sorry. For that, I'd recommend Double or E(2/4/6).

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 3, 2016

Okay, I'll start with Double for those then, thanks.

alexeyzab added some commits Sep 3, 2016

Add prefixes to all datatypes
Also start using hindent for formatting the code.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 4, 2016

@bitemyapp Hey Chris! I've added a rough draft of a mkRequest function to the Client. I am following the structure of the dispatch function from Bloodhound, planning on adding the type aliases and other things later. Trying to do the bare minimum first so I can understand what's going on.

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 4, 2016

Another next step would be to make a more "typed" API that takes a request/query datatype as input, performs the HTTP request, and returns a response type that has parsed the JSON and turned that into an explicit Haskell datatype already.

I wouldn't necessarily do that next as I think tests are more important for now. I can save you time on the typed API return value thingy by showing you some tactics in Bloodhound and stripe-haskell.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 4, 2016

Right, I'll start working on tests then.

I see, that's what I had in mind as well, the JSON response for this particular endpoint should be parsed properly into a RateResponse datatype. I am not yet sure how to generalize that, should mkRequest delegate the decoding to a separate function that works with the particular endpoint or is there some better way to do this?

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 4, 2016

I am not yet sure how to generalize that

That's the bit I thought I'd save you time on, give you a few suggestions/options. When can you hop into a vid call?

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 4, 2016

Ahh, got it. I can do that now, if you are free.

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 4, 2016

@alexeyzab sent you link in DMs

Add hspec tests
First draft of the "/rate" endpoint test.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 6, 2016

I've added one hspec test so far. It makes sure the decoded responseBodyis set properly. There is a lot more JSON that can be returned here. I think it has to do with setting up the ShipWire info for the items you have. I will look into that.

alexeyzab added some commits Sep 7, 2016

Add type families for /rate endpoint
Change the single spec to use the new `dispatch` function.
The spec is not returning the fullest JSON possible because of the
ShipWire account limitations. Emailed the support about that.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

@bitemyapp Hey Chris! I am starting to use the type families the way they are done in the Stripe API wrapper. Got a question though.

What's the purpose of this empty datatype?
I noticed how it's used in here, createCustomer looks like this.

I couldn't get a similar setup work with my dispatch function because there is no FromJSON defined for CreateRateResponse. So instead I did this:

type instance ShipWireReturn RateResponse = RateResponse

Not sure if that's a right solution.

As for the hspec tests, I think I need to get my sandbox approved so I can get the full JSON response from ShipWire, I've emailed their support, this hasn't been resolved yet.

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 8, 2016

@alexeyzab

What's the purpose of this empty datatype?

It be a request type that points at Customer

Usually the argument type is an empty data decl and the result should be representational.

So it would be something like:

data DateRequest
type instance ShipWireReturn RateRequest = RateResponse

-- then RateResponse is a real datatype with a FromJSON
Add the request datatype
This is more inline with stripe-haskell now.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

I think I get it now. Thanks, Chris!

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 8, 2016

@alexeyzab a request: could you add a link in the comments to the Shipwire API being implemented next to each request type?

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

@bitemyapp will do.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

@bitemyapp something like this?

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

Also, should we merge this so I can move on to other endpoints or is there anything I should add here first?

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 8, 2016

@alexeyzab there are some ways in which I'd amend it. One is that I'd not make the endpoint the client uses accept a Maybe ByteString body, I'd make them pass a datatype which can get serialized for the HTTP client. (I mean this function here:

createRateResponse :: Maybe BSL.ByteString -> ShipWireRequest CreateRateResponse
)

For a contrary example, there's the StripeRequest type which is essentially what the http client needs to immediately fire a request: http://hackage.haskell.org/package/stripe-core-2.1.0/docs/Web-Stripe-StripeRequest.html#t:StripeRequest

But the part the user directly uses is stuff like this: http://hackage.haskell.org/package/stripe-core-2.1.0/docs/Web-Stripe-Card.html#v:createCustomerCardByToken where they pass in structured data and something you can submit to Stripe via the Stripe client comes out the other side. Be very suspicious of any raw text or bytestring types exposed to your user.

Another is that I'd change the name of request type: CreateRateResponse to something more semantic, like RateQuote or RateRequest and then the reply is something like RateQuoteResponse or RateResponse.

Does all this make sense? I agree this should get merged soon. If you're alright making these changes, we'll move to a merge and I'll look at the testing story this weekend.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

Ah, so instead of doing Maybe BSL.ByteString, it would be Rate -> ShipWireRequest RateResponse (maybe rename Rate to RateSetup?). I think I understand.

Will change the names too. Yeah, it makes sense, I'll go ahead and try implementing all of that now. Thank you!

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 8, 2016

@alexeyzab sounds like you're on the right track.

I hate nitpicking over names, but they can guide people to the right intuition without needing to check the docs, so I do care a bit.

for Rate/RateSetup, if we were mimicking stripe-haskell then I think it'd be GetRate or GetRateQuote. I leave it to you to decide what makes the most sense.

The name nitpicking is because it's not clear just from Rate what the action does unless you've spent a lot of time with fulfillment APIs. Knowing you're constructing a hypothetical shipment to get a quote is what we're trying to convey, I think.

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 8, 2016

@bitemyapp I understand it's important to have decent names, I'll try to be more descriptive in the future. Thank you!

Think I've done what you've mentioned. Along the way I realized that GetRate is not optional for that endpoint so I switched from Maybe GetRate to GetRate. Because otherwise we would get an error from the API.

alexeyzab added some commits Sep 8, 2016

Add more descriptive names
Also get rid of passing a Maybe Lazy Bytestring straight to
`createRateRequest` and instead pass in a non-optional GetRate.
Rename types, add a missing datatype
Turns out despite having a `type` field inside the `warnings` array we
can actually have a separate array altogether called `errors`.
@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 14, 2016

@bitemyapp Hey Chris! Is there anything else I should add to this pull request? I've started working on another endpoint in a different branch, should we merge this one in?

@bitemyapp

This comment has been minimized.

Collaborator

bitemyapp commented Sep 14, 2016

@alexeyzab I'll merge it, you start work in a new branch, I'll give this another look this weekend.

@bitemyapp bitemyapp merged commit 33479aa into alexeyzab:master Sep 14, 2016

@alexeyzab

This comment has been minimized.

Owner

alexeyzab commented Sep 14, 2016

Yep, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment