[SupersetClient] implement put and delete HTTP methods #85
Conversation
mistercrunch
force-pushed
the
requests
branch
from
6f92572 to
e8fd133
Compare
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 68 68
Lines 774 780 +6
Branches 182 180 -2
=====================================
+ Hits 774 780 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for working to add these, initially we did not because these methods weren't used elsewhere in the app.
Had a few suggestions, I'm not a big fan of adding the general request on the public API and I think there's still work to update the existing POST implementation, but can see if @kristw or @xtinec think otherwise.
| @@ -34,6 +35,7 @@ const SupersetClient: SupersetClientInterface = { | |||
| isAuthenticated: () => getInstance(singletonClient).isAuthenticated(), | |||
| post: (request: RequestConfig) => getInstance(singletonClient).post(request), | |||
| reAuthenticate: () => getInstance(singletonClient).init(/* force = */ true), | |||
| request: (request: RequestConfig) => getInstance(singletonClient).get(request), | |||
There was a problem hiding this comment.
I don't think we should expose a general request method on the public interface, there are only 4 possible methods so if you want delete or put I would just add those.
There was a problem hiding this comment.
Looks like there are many more methods: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods
Happy to add more explicit delete and put as they are most common. Can I leave request just in case? If someone has an edge case where they need to use one of the more obscure methods, it sucks to have to come and add it here...
| @@ -70,12 +70,17 @@ export class SupersetClientClass { | |||
| return this.csrfToken !== null && this.csrfToken !== undefined; | |||
| } | |||
|
|
|||
| async get({ | |||
| async get(requestConfig: RequestConfig): Promise<SupersetClientResponse> { | |||
There was a problem hiding this comment.
if you're refactoring get like this, I think you should also refactor post, and then could add delete / put on top of those.
|
@kristw @williaster this is ready for review, let me know if you want me to withdraw |
mistercrunch
changed the title
There was a problem hiding this comment.
LGTM, thanks for adding these! 🏆
for publishing, we could handle this release, or we can get you set up with npm publishing. if you want to do it, can you give me your npm username / make one and I'll add you to the @superset-ui org? mistercrunch didn't seem to exist. after that we can chat on slack for more detailed instructions.
|
@williaster I just created an account here: https://www.npmjs.com/~mistercrunch |
|
sent an invite 👌 Sounds like this should be included in the release @kristw just made. |
…#85) Updates the requirements on [@types/d3-array](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/d3-array) to permit the latest version. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/d3-array) Signed-off-by: dependabot[bot] <support@dependabot.com>
🏆 Enhancements
Trying to use METHOD = DELETE