Skip to content
This repository was archived by the owner on May 22, 2025. It is now read-only.

DisputableVoting: Implement connector models#181

Merged
facuspagnuolo merged 5 commits intomasterfrom
disputable_voting_connector
Aug 5, 2020
Merged

DisputableVoting: Implement connector models#181
facuspagnuolo merged 5 commits intomasterfrom
disputable_voting_connector

Conversation

@facuspagnuolo
Copy link
Copy Markdown
Contributor

MERGE AFTER #178

@facuspagnuolo facuspagnuolo requested review from 0xGabi and bpierre August 3, 2020 23:17
@facuspagnuolo facuspagnuolo self-assigned this Aug 3, 2020
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 3, 2020

Codecov Report

Merging #181 into master will increase coverage by 5.42%.
The diff coverage is 62.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   27.14%   32.57%   +5.42%     
==========================================
  Files          81       96      +15     
  Lines        1477     1747     +270     
  Branches      251      273      +22     
==========================================
+ Hits          401      569     +168     
- Misses       1076     1178     +102     
Flag Coverage Δ
#unittests 32.57% <62.22%> (+5.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t-voting-disputable/src/models/DisputableVoting.ts 2.43% <2.43%> (ø)
packages/connect-voting-disputable/src/connect.ts 40.00% <40.00%> (ø)
...onnect-voting-disputable/src/thegraph/connector.ts 47.94% <47.94%> (ø)
...oting-disputable/src/thegraph/parsers/castVotes.ts 66.66% <66.66%> (ø)
...s/connect-voting-disputable/src/models/CastVote.ts 73.33% <73.33%> (ø)
...isputable/src/thegraph/parsers/disputableVoting.ts 80.00% <80.00%> (ø)
...t-voting-disputable/src/thegraph/parsers/voters.ts 83.33% <83.33%> (ø)
...voting-disputable/src/thegraph/parsers/settings.ts 85.00% <85.00%> (ø)
...ct-voting-disputable/src/thegraph/parsers/votes.ts 86.66% <86.66%> (ø)
...kages/connect-voting-disputable/src/models/Vote.ts 92.30% <92.30%> (ø)
... and 20 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 a3cd199...5599bf1. Read the comment docs.

Copy link
Copy Markdown
Contributor

@0xGabi 0xGabi left a comment

Choose a reason for hiding this comment

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

I left a few comments 👍

Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
Comment thread packages/connect-voting-disputable/src/thegraph/connector.ts Outdated
return data.dao
}

onDao(callback: Function): SubscriptionHandler {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder in which cases we may have a subscription to onDao? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was kind of inclined to do this due to Pierre's suggestion (see here), however, maybe we can avoid those that may not be necessary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, cool, I think we can avoid some of them 👍

import { SettingData } from '../../types'

function buildSetting(setting: any, connector: any): Setting {
const {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we may use spread syntax to build the settingData we need?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not sure I follow :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean something like:

const settingData = {
    ...setting,
    votingId: setting.voting.id
}

But maybe if we do that we break the SettingData interface 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it breaks it cause we will include some objects like setting.voting that are not part of the interface, I think at some point we need to do some filtering.

What we could do is to use Object.assign in the entity constructor since it is receiving an object that implements the expected interface

Base automatically changed from disputable_voting_subgraph to master August 4, 2020 19:11
facuspagnuolo and others added 2 commits August 4, 2020 16:13
Copy link
Copy Markdown
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@facuspagnuolo facuspagnuolo merged commit 28b0bd6 into master Aug 5, 2020
@facuspagnuolo facuspagnuolo deleted the disputable_voting_connector branch August 5, 2020 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants