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

Use typeorm for accounts fetching #164

Merged
merged 14 commits into from
May 20, 2019
Merged

Conversation

charlie-wasp
Copy link
Contributor

@charlie-wasp charlie-wasp commented May 13, 2019

Big time! This PR makes use of the typeorm to simplify database related stuff. As a POC I used it only for accounts for now.

So far it seems that it can help us to reduce number of types (we won't need separate interface for table rows) and to retire fromDb method from factories. Also we won't need squeal package for building queries


This change is Reviewable

@charlie-wasp charlie-wasp requested a review from a team May 13, 2019 09:29
@charlie-wasp
Copy link
Contributor Author

Now this PR also addresses #157

@charlie-wasp charlie-wasp force-pushed the play/accounts-typeorm branch from 8e7ef33 to 70b7934 Compare May 13, 2019 15:35
@charlie-wasp charlie-wasp marked this pull request as ready for review May 13, 2019 16:09
@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #164 into master will increase coverage by 0.52%.
The diff coverage is 67.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage    47.8%   48.32%   +0.52%     
==========================================
  Files         121      120       -1     
  Lines        1910     1916       +6     
  Branches      231      230       -1     
==========================================
+ Hits          913      926      +13     
+ Misses        997      990       -7
Impacted Files Coverage Δ
src/schema/resolvers/index.ts 100% <ø> (ø) ⬆️
src/database.ts 100% <ø> (ø) ⬆️
src/model/factories/index.ts 100% <ø> (ø) ⬆️
src/model/signer.ts 88.88% <ø> (-1.12%) ⬇️
src/schema/accounts.ts 100% <ø> (ø) ⬆️
src/model/factories/account_values_factory.ts 46.15% <0%> (ø) ⬆️
src/model/factories/signer_factory.ts 21.42% <0%> (+2.67%) ⬆️
src/orm/entities/account_data.ts 100% <100%> (ø)
src/schema/resolvers/transaction.ts 16.66% <100%> (-1.86%) ⬇️
src/util/paging.ts 31.81% <37.5%> (-5.69%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc4cfdd...3a6ebb3. Read the comment docs.

@charlie-wasp charlie-wasp force-pushed the play/accounts-typeorm branch from 3c4def2 to bc4b513 Compare May 14, 2019 09:44
Copy link
Member

@nebolsin nebolsin left a comment

Choose a reason for hiding this comment

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

Reviewed 35 of 35 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @charlie-wasp)


tsconfig.json, line 21 at r1 (raw file):

    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": false,

I think you can selectively opt-out from property initialization check:

"strict": true,
"strictPropertyInitialization": false

Btw, why do we want to skip this check?

@charlie-wasp charlie-wasp requested a review from nebolsin May 20, 2019 13:16
Copy link
Contributor Author

@charlie-wasp charlie-wasp left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nebolsin)


tsconfig.json, line 21 at r1 (raw file):

Previously, nebolsin (Sergey Nebolsin) wrote…

I think you can selectively opt-out from property initialization check:

"strict": true,
"strictPropertyInitialization": false

Btw, why do we want to skip this check?

We want it because typeorm entities declaration uses properties without initialization, so project can't be compiled with this option enabled

Copy link
Member

@nebolsin nebolsin left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


tsconfig.json, line 21 at r1 (raw file):

Previously, charlie-wasp (Timur Ramazanov) wrote…

We want it because typeorm entities declaration uses properties without initialization, so project can't be compiled with this option enabled

Ok, although I think it could also be mitigated by defining TypeORM properties as <type> | undefined, which might be a better option. I’m fine with disabling the check for now, so it’s up to you.

@charlie-wasp charlie-wasp force-pushed the play/accounts-typeorm branch from 68decc0 to a33f247 Compare May 20, 2019 13:39
@charlie-wasp charlie-wasp force-pushed the play/accounts-typeorm branch from a33f247 to e3b7769 Compare May 20, 2019 14:43
@charlie-wasp charlie-wasp merged commit e233d65 into master May 20, 2019
@charlie-wasp charlie-wasp deleted the play/accounts-typeorm branch May 20, 2019 15:20
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.

2 participants