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.

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

Make servant-foreign code nicer #372

Merged

Conversation

dredozubov
Copy link
Member

I felt like i have to do this before addressing a few other issues with servant-foreign

Basically it's:

  • non-messy imports
  • got rid of most long lines (>80 chars)
  • prisms for sum types and newtypes(we use lens anyway, so why not)
  • consistent indentation

Also i made some changes to demonstrate my point from #345 (comment)

Review on Reviewable

@dredozubov
Copy link
Member Author

If you too feel like it looks more readable this way, maybe i'll go through other packages with this PR

@fizruk
Copy link
Member

fizruk commented Feb 11, 2016

Nice! 👍

Would be even nicer if you add a proper haddock documentation!
Would be magical if you also document design choices and intended usage!

No pressure though :)

@dredozubov
Copy link
Member Author

@fizruk, i plan to do that after i'll properly address #351 ;) After some consideration i feel like there may be some design changes on its way.

* non-messy imports
* got rid of most long lines (>80 chars)
* prisms for sum types and newtypes(we use lens anyway, so why not)
* consistent indentation
@purcell
Copy link
Contributor

purcell commented Feb 12, 2016

This is all great, but it'd be a big job to rebase #351 on top of it. Let me know if you want a hand with anything.

@jkarni
Copy link
Member

jkarni commented Feb 16, 2016

👍 - @dredozubov can we merge?

@dredozubov
Copy link
Member Author

@jkarni sure, but i also wonder if it's worth doing repo-wise. I'd be more than happy to see it, but it contradicts the

Though we aren't sticklers for style

from https://github.com/haskell-servant/servant/blob/master/CONTRIBUTING.md

@dredozubov
Copy link
Member Author

@purcell i started doing servant-rust to test the design, but my day work consumes the most of my time, so i kinda feel that i don't want to rush #351, it can always land in 0.5.1 or something

dredozubov added a commit that referenced this pull request Feb 17, 2016
@dredozubov dredozubov merged commit 8dc7328 into haskell-servant:master Feb 17, 2016
@dredozubov dredozubov deleted the stylish-servant-foreign branch February 17, 2016 08:46
@purcell
Copy link
Contributor

purcell commented Feb 17, 2016

Fair enough, and this is a nice tidy-up. Do you think it'd be helpful if I try to rebase #351 on top of this now? Or do you already know my approach there wouldn't be optimal?

@dredozubov
Copy link
Member Author

@purcell i'll be able to tell a bit later, don't hurry with rebasing it. Are you on irc btw?

@purcell
Copy link
Contributor

purcell commented Feb 17, 2016

i'll be able to tell a bit later, don't hurry with rebasing it. Are you on irc btw?

Ah, no, sorry, but I'm on github a lot and happy to discuss. And happy to pop onto IRC at some point if it'd help.

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.

None yet

4 participants