Skip to content

Conversation

@mdimjasevic
Copy link
Contributor

This is work in progress and is not supposed to be merged yet.

Issue: https://iohk.myjetbrains.com/youtrack/issue/TSD-85

@ksaric ksaric added the WIP label Jul 26, 2018
@ksaric ksaric self-requested a review July 26, 2018 11:00
, bytestring
, containers
, directory
, either
Copy link
Contributor

Choose a reason for hiding this comment

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

You really don't need this. Try it without.

Copy link
Contributor

@ksaric ksaric left a comment

Choose a reason for hiding this comment

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

Try to focus the changes on the HTTPNetworkLayer first, then you can start with the next step.

src/HttpLayer.hs Outdated
import Universum

import Data.Aeson (FromJSON, ToJSON, Value, encode)
import Control.Exception.Safe
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect formatting. You can use stylish-haskell to format imports, the general guide you can follow is here - https://github.com/serokell/serokell-util/blob/master/serokell-style.md

-- | The connection pooled Zendesk layer. Used for database querying.
-- We need to sync occasionaly.
connPoolDataLayer :: forall m. (MonadBaseControl IO m, MonadIO m, MonadReader Config m) => DBConnPool -> DataLayer m
connPoolDataLayer :: forall m. (MonadBaseControl IO m, MonadIO m, MonadReader Config m, MonadCatch m) => DBConnPool -> DataLayer m
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that constraint here?

-- - list returns multiple results
-- - post submits a result (maybe PUT?!)
basicDataLayer :: (MonadIO m, MonadReader Config m) => DataLayer m
basicDataLayer :: (MonadIO m, MonadReader Config m, MonadCatch m) => DataLayer m
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it here?

Right req'' -> do
req''' <- apiCall req''
case req''' of
Right (PageResultList pagen nextPagen count) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

http://hackage.haskell.org/package/base-4.6.0.0/docs/src/Data-Either.html#Either - Monad (Either e)
Please apply in other places where applicable.

src/HttpLayer.hs Outdated
------------------------------------------------------------
-- JSON parsing exceptions
------------------------------------------------------------
-- | Exceptions that occur during JSON parsing
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want comments in the codebase, people forget about this.

@ksaric
Copy link
Contributor

ksaric commented Aug 9, 2018

Guys, focus on HTTPNetworkLayer only - we don't need all the other changes for now (and you will have to change them anyway).
I don't want you to focus on all the other layer, but just the one I asked you to - otherwise we are doing too much work per PR, and making things more complicated to review, alongside that focusing on that single layer I can actually show you the transformations you need to get to the desired code. Otherwise, it will be too much back and forth with the changes, explanations and so on.

Look for accuracy, speed will come in time.

@mdimjasevic
Copy link
Contributor Author

@ksaric , there might be changes in e.g. DB.hs that are leftovers from our early wandering days. Other than that, aren't we here making changes only to the HTTP layer?

@ksaric
Copy link
Contributor

ksaric commented Aug 9, 2018

@mdimjasevic Start with HTTPNetworkLayer only - we can move from there on out.

@ksaric
Copy link
Contributor

ksaric commented Aug 28, 2018

Closing in favor of #57

@ksaric ksaric closed this Aug 28, 2018
@ksaric ksaric deleted the mdimjasevic/tsd-85-http-exceptions branch August 28, 2018 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants