-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
It also now contains the name and network properties.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I was wondering if you saw code using the inherited 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 In any case, I 100% agree that we should document any breaking change (or any other change 😄) in the public API 👍 |
No totally, you are right. I got confused by the inheritance. Developers are using the |
It also now contains the
name
andnetwork
properties.