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

Add enumeratum-quill #170

Merged
merged 8 commits into from
Feb 13, 2018
Merged

Add enumeratum-quill #170

merged 8 commits into from
Feb 13, 2018

Conversation

daniel-shuy
Copy link
Contributor

As mentioned in #163.

Adds QuillEnum trait to automatically convert EnumEntrys to/from their entryName.
Adds QuillValueEnum trait to automatically convert ValueEnumEntrys to/from their values.

I wasn't sure what to put for the version, so I left it as 0.1.0-SNAPSHOT.

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

LGTM. Do you mind fixing the merge conflict with master?

)
}
)
lazy val enumeratumQuillJs = enumeratumQuill.js
Copy link
Owner

Choose a reason for hiding this comment

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

TIL: Quill works with ScalaJS O_o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha I was shocked as well

@daniel-shuy
Copy link
Contributor Author

Just discovered that all Quill contexts don't have an Encoder/Decoder for Char, so I had to convert CharEnum to String instead.

@daniel-shuy
Copy link
Contributor Author

I've added a note on them in the README

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 16869e1 on daniel-shuy:master into 6046611 on lloydmeta:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 16869e1 on daniel-shuy:master into 6046611 on lloydmeta:master.


ctx.run(query[Shirt]).foreach(println)
```
- Note that an explicit cast to the `ValueEnumEntry` abstract class (eg. `ShirtSize.Small: ShirtSize`) is required when binding hardcoded `ValueEnumEntry`s
Copy link
Owner

Choose a reason for hiding this comment

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

No big deal; but this isn't an explicit cast; just an explicit type annotation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I knew that it wasn't an explicit cast (asInstanceOf), but I forgot what it was called, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if we were to be pedantic, I guess this should be called a Type Ascription instead of a Type Annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, you missed out on one of the "explicit cast"s. I've opened a PR (#172) to fix both.

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

Re-reviewing, still LGTM :) Will merge later today.

@lloydmeta lloydmeta merged commit eefef9e into lloydmeta:master Feb 13, 2018
@lloydmeta
Copy link
Owner

Hey thanks for this ! Just merged and released it as 1.5.12. It should be on Maven Central soonish.

Just a note: apparently Quill is only 2.11 + so 2.10 isn't supported :)

Great work and thanks again!

@daniel-shuy
Copy link
Contributor Author

Oops, I forgot to run test with crossScalaVersion (+ test). I'm surprised the CI passed O_o

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.

3 participants