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

Rename ConnectorInterface to IOrganizationConnector #137

Merged
merged 2 commits into from
Jul 21, 2020

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented Jul 14, 2020

It also now contains the name and network properties.

It also now contains the name and network properties.
@bpierre bpierre requested a review from 0xGabi July 14, 2020 15:24
@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #137 into master will increase coverage by 0.17%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   23.92%   24.09%   +0.17%     
==========================================
  Files          59       59              
  Lines        1049     1054       +5     
  Branches      168      167       -1     
==========================================
+ Hits          251      254       +3     
- Misses        798      800       +2     
Flag Coverage Δ
#unittests 24.09% <54.54%> (+0.17%) ⬆️
Impacted Files Coverage Δ
...ages/connect-core/src/connections/ConnectorJson.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/App.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/CoreEntity.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Organization.ts 0.00% <ø> (ø)
packages/connect-core/src/entities/Permission.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Repo.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Role.ts 0.00% <0.00%> (ø)
packages/connect-core/src/index.ts 0.00% <ø> (ø)
packages/connect-thegraph/src/connector.ts 60.41% <76.47%> (+2.63%) ⬆️
packages/connect-thegraph/src/parsers/apps.ts 92.59% <100.00%> (ø)
... and 1 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 b9cfe5f...6456de9. Read the comment docs.

@bpierre bpierre mentioned this pull request Jul 14, 2020
9 tasks
Copy link
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.

Great work with the renaming! 🙌

My only concern is with the backward compatibility issue we will introduce moving GraphQLWrapper to #gql. I think is the right decision but we should clearly state that the next version has breaking changes.

)
}
super(_orgSubgraphUrl, verbose)

this.#gql = new GraphQLWrapper(orgSubgraphUrl, config.verbose)
Copy link
Contributor

@0xGabi 0xGabi Jul 14, 2020

Choose a reason for hiding this comment

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

Nice!

@bpierre
Copy link
Contributor Author

bpierre commented Jul 14, 2020

My only concern is with the backward compatibility issue we will introduce moving GraphQLWrapper to #gql. I think is the right decision but we should clearly state that the next version has breaking changes.

I was wondering if you saw code using the inherited GraphQLWrapper methods? I was considering it to be an internal API.

In terms of breaking changes, I was more thinking about the constructor params of the connectors. But I don’t see a reason to instantiate ConnectorTheGraph manually since it’s embedded, so chances are that people won’t have to make any change here.

In any case, I 100% agree that we should document any breaking change (or any other change 😄) in the public API 👍

@0xGabi
Copy link
Contributor

0xGabi commented Jul 15, 2020

No totally, you are right. I got confused by the inheritance. Developers are using the GraphQLWrapper directly and not from the OrganizationConnector so should no be backward compatibility issues other than what you mention about constructor params.

@bpierre bpierre merged commit 1a6e7cb into master Jul 21, 2020
@bpierre bpierre deleted the connector-interface-renaming branch July 21, 2020 14:51
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