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 `DB::Serializable` #115

Merged
merged 5 commits into from Oct 31, 2019
Merged

Add `DB::Serializable` #115

merged 5 commits into from Oct 31, 2019

Conversation

@nickbclifford
Copy link
Contributor

nickbclifford commented Oct 27, 2019

This PR adds the DB::Serializable module and corresponding DB::Field annotation in analogue with stdlib JSON::Serializable. Resolves #105.

@bcardiff bcardiff merged commit e1832fc into crystal-lang:master Oct 31, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Oct 31, 2019

Thanks for the neat implementation!

@nickbclifford nickbclifford deleted the nickbclifford:serializable-annotation branch Oct 31, 2019
@pynixwang

This comment has been minimized.

Copy link

pynixwang commented Jan 16, 2020

Model.from_rs should return only one instance.
Array(Model).from_rs return an array is better

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 16, 2020

@bcardiff I agree with @pynixwang here, what do you think about making this change?

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Jan 16, 2020

It bugged me in the past the distinction between Model.new(rs) and Model.from_rs(rs). So yeah, we can change the design.

There was no decision in this PR, it was based on the current desing of DB::Mapping.

A nice thing to achieve/keep is that the db.query_all|one|each("...", as: Model) work yielding or returning the appropriate type. I thing query_each might be missing. I think those are the main API and from_rs is more an internal thing.

@pynixwang what pushed you to use directly from_rs?

@pynixwang

This comment has been minimized.

Copy link

pynixwang commented Jan 16, 2020

@bcardiff use like JSON.

or from_db?

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Jan 16, 2020

Can I get more details? :-)

query_all is already something that returns Array. So I see little benefit in having an internal like from_rs to do that also.

To iterate each entity directly you could either avoid having a rs at all by using db.query_...("...", as: Model). Or at least rs.read(Model) thanks to this definition. (DB::Serializable includes DB::Mappable).

So I see no need to explicitly call from_rs. Moving this conversation to an internal design discussion (which are easier to act upon if they don't change the main API).

@pynixwang

This comment has been minimized.

Copy link

pynixwang commented Jan 16, 2020

ok, I see.
not using it in project cause a mistake.

@pynixwang

This comment has been minimized.

Copy link

pynixwang commented Jan 16, 2020

@bcardiff the doc lead me to a black hole

http://crystal-lang.github.io/crystal-db/api/0.8.0/DB/Serializable.html

employees = Employee.from_rs(db.query("SELECT title, name FROM employees"))
@pynixwang

This comment has been minimized.

Copy link

pynixwang commented Jan 16, 2020

this doc must be changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.