search

fpco / libraries

FP Complete libraries mega-repo
2 stars 0 forks source link

Use monad-logger-json-extra #188

Closed borsboom closed 8 years ago

bitonic commented 8 years ago

I will not be able to attend this PR since I'm leaving for my holidays soon. I think one of @kantp , @snoyberg , or @nh2 should review.

kantp commented 8 years ago

@borsboom: I can't find the commit d7a5495c4b7dd0e38bf7e56b1210aaccb91d35ff does not exist in fpco/monad-logger-json-extra.

borsboom commented 8 years ago

Ah sorry I missed updating the git commit in the libraries and hauth's stack.yaml. Fixed now, and the correct commit is 931a096f4d18a57430908ec65a87556e26c5d298.

borsboom commented 8 years ago

Since @kantp isn't available today, perhaps someone else can review? @nh2 or @snoyberg?

kantp commented 8 years ago

Looks good to me. Only question I'd have: at some places, the type of the logging message is fixed to String, in others to Text. Unless there's a reason for using String, I'd suggest using Text everywhere.

borsboom commented 8 years ago

I went by context. If Text was already imported and in use by the module, I used it. Otherwise I went with String. But you're right, no good reason for this and might as well just use Text everywhere.

kantp commented 8 years ago

I guess it doesn't really matter, I just noticed it and wondered if there was a reason.

Feel free to merge.