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

Code review #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Code review #6

wants to merge 4 commits into from

Conversation

V-Lamp
Copy link

@V-Lamp V-Lamp commented Apr 26, 2017

Overall, it is a well tested and library providing good type-safety and convenience to the user.

I liked the testing of the type classes with ScalaCheck and Discipline, while also having concrete test cases with manual sample data, acting also as a guidance of the intended usage of the methods.

In terms of improvements to the existing form of the library, I would:

  • Provide more clarity on the use-case of the two apply methods on Encoder and Decoder, possibly reducing them to one.

  • The usage of Shapeless is impressive but rather complicated. I would like to see more concrete use cases for this and maybe simplify it by separating concerns to make it more approachable to users.

Ideas/suggestions on improving it:

  • I found it a bit hard to find what is the main added value of the library (a generically derived DynamoDBSerializer). I would like to make the code lead more naturally to this.

  • Using Shapeless: I would investigate the usage of LabelledGeneric and annotating the keys with the type symbol instead of String name of the column. This might also simplify the type-level derivation since it could match case class field to the Key tagged with the same label.

  • Possibly use SourceCode to get the key names when explicitly assigning them

  • Allow encoding/decoding of nested case classes

V-Lamp and others added 4 commits April 21, 2017 22:40
To avoid unrelated changes in later commits.
…es for further reviewing.

Encoder[Option] now encodes None as AttributeValue null representation.
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.

1 participant