mastarija / valor

Simple and powerful data validation library with Applicative and Monad interface.
MIT License
18 stars 1 forks source link

Feedback + code review #1

Closed chshersh closed 6 years ago

chshersh commented 6 years ago

Since GitHub doesn't support submission of code review outside Pull Request I will write my thoughts under this issue (as I promised earlier on Reddit).

Good parts

  1. Hackage documentation. Documentation is really good and clear! I like examples on Hackage.

Can be improved: You can add links to your functions inside inner code blocks to visualize functions exactly from your package. So instead of this:

> articleValidator :: Monad m => Validator Article m ArticleError
> articleValidator = Article
>   <$> check        id      over18
>   <*> checks       title   [nonempty, nonbollocks]

You can write:

> articleValidator :: Monad m => 'Validator' Article m ArticleError  -- note single quotes
> articleValidator = Article
>   <$> 'check'        id      over18
>   <*> 'checks'       title   [nonempty, nonbollocks]

This will make docs slight more readable (at least for my taste). Also, instead of writing > in front of every line, you can just write @ at the beginning and at the end, like this:

@
articleValidator :: Monad m => Validator Article m ArticleError
articleValidator = Article
  <$> check        id      over18
  <*> checks       title   [nonempty, nonbollocks]
@
  1. Readme. I like comparison with other libraries and motivation!

Can be improved: Probably make separate explicit section for comparison? Also I think it's good to add link to hackage. At least via Hackage badge README shield.

  1. Code. It's really clean and understandable. Style and formatting is good. I also appreciate explicit imports!

Possibly can be better (opinionated)

  1. Some CI will be good to have in project.
  2. Opinionated. I would personally not reexport ExceptT and that stuff. From beginner view it's better to see origin modules for those types in code. Also, it won't make transformers package hidden dependency on Hackage which is a nice thing to have.
  3. Alternative instance. I don't think it's possible to add it. But could be really great to have if you have multiple alternative ways to validate data. Not sure if it's an actual use case. But will be good for supporting sum types.
  4. Tests. They are not written as I can see. And I would recommend hedgehog library instead of QuickCheck for property-based testing. Though, it seems quite a challenge to come with the properties for this library...
  5. Literate Haskell. You documentation can become outdated after you change something in library. So you can put tutorial inside README.md file and use combination of Literate Haskell + markdown-unlit package to make your tutorial actually compilable. This will ensure that it will always be up-to-date.
reygoch commented 6 years ago

@ChShersh thank you for the feedback, I'm implementing some of the changes you have suggested. I hope I can bother you to take another look once I'm done with it :D