-
Notifications
You must be signed in to change notification settings - Fork 9
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
chore(capabilities): define native query connection capabilities #1211
Conversation
a11335c
to
4c85e0a
Compare
Code Climate has analyzed commit 54eb825 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (98% is the threshold). This pull request will bring the total coverage in the repository to 97.2% (0.0% change). View more on Code Climate. |
How is the dev supposed to declare that his datasource supports native queries in this PR ? 🙏 |
@@ -62,4 +75,18 @@ export default class CompositeDatasource<T extends Collection = Collection> | |||
|
|||
throw new Error(`Chart '${name}' is not defined in the dataSource.`); | |||
} | |||
|
|||
async executeNativeQuery( |
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.
Is there a way to test this at the moment ?
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.
The composite datasource implementation is already tested, only the real execution is not yet done as it is bound to change, I simply made the closest implementation I had in mind for it
return this.dataSources | ||
.find(datasource => Boolean(datasource.nativeQueryConnections[connectionName])) | ||
.executeNativeQuery(connectionName, query, contextVariables); | ||
} |
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.
why not this ?
return this.dataSources | |
.find(datasource => Boolean(datasource.nativeQueryConnections[connectionName])) | |
.executeNativeQuery(connectionName, query, contextVariables); | |
} | |
return this.nativeQueryConnections[connectionName] | |
.executeNativeQuery(connectionName, query, contextVariables); | |
} |
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.
the datasource has the implementation of the executeNativeQuery
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.
-
✅ PR title
-
✅ Destination branch
-
✅ PR linked to the clickup task
-
ℹ️ Code review -> small question
- ✅ Use pure functions when possible
- ✅ Performance concerns
- ✅ Security concerns
- ✅ The PR tackle one subject only
-
✅ Automatic tests
- ✅ Unit tests
- ⤬ Integration tests
-
✅ Manual tests
- ℹ️ Test the functionality. Did not the query execution
- ⤬ Test error cases
🎉 This PR is included in version 1.12.6 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.11.14 🎉 The release is available on Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.53.2 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.55.5 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.1.40 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.5.4 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.4.8 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.1.13 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.3.8 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.3.26 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Definition of Done
General
Security