fix: change constructServiceURL from method to function #141
Conversation
Now it can be used as a utility function instead of an inherited method. An SDK class can now include a `constructServiceURL` method with a different signature than this function.
lib/helper.ts
Outdated
| * the default variable value will be used instead. | ||
| * @returns {string} The formatted URL with all variable placeholders replaced by values. | ||
| */ | ||
| export function constructServiceURL( |
There was a problem hiding this comment.
I only just now thought of this, but since we're changing it already... we don't fully capitalize "URL" elsewhere in the core. For example, the BaseService class has a method called setServiceUrl. I think we probably want to be consistent and call this constructServiceUrl. Sorry for not catching that earlier!
There was a problem hiding this comment.
I noticed this too, and tbh we have the same discrepancy in the java core. I only noticed that AFTER the PR was merged in :)
Let's make this change here while we're changing things anyway.
There was a problem hiding this comment.
BTW, I'm not suggesting we change the Java core :)
test/unit/parameterized-url.test.js
Outdated
| @@ -1,4 +1,4 @@ | |||
| const { BaseService } = require('../../dist/lib/base-service'); | |||
| const { constructServiceURL } = require('../../dist/lib/helper'); | |||
There was a problem hiding this comment.
Question: will moving this function to helpers.ts make it more difficult to import it in the generated SDK code?
@dpopp07 any idea?
There was a problem hiding this comment.
@padamstx Good question - no, it won't. All of the helper methods are exported at the top level of the core so the import will be simple:
const { constructServiceUrl, <any-other-imports> } = require('ibm-cloud-sdk-core');There was a problem hiding this comment.
ok, sounds good, but I assume this means that when we generate the service code for a service, we'll need to be careful about correctly importing constructServiceUrl from the core when we will be generating a service-specific constructServiceUrl() function that will reference the function in the core.
There was a problem hiding this comment.
Oh, because of name conflicts? That could be a legitimate issue, though it should be easy to get around in a compatible way
There was a problem hiding this comment.
I wasn't thinking about name conflicts actually. But in the node generator, when we process an API definition, we'd generate the constructServiceUrl() function in the service .ts file ONLY if we detect server variables in one of the "servers" entries.
So only in that case would we want to import the "constructServiceUrl" function from the core, since that's the only scenario where we'd use it.
lib/helper.ts
Outdated
| * the default variable value will be used instead. | ||
| * @returns {string} The formatted URL with all variable placeholders replaced by values. | ||
| */ | ||
| export function constructServiceURL( |
There was a problem hiding this comment.
I noticed this too, and tbh we have the same discrepancy in the java core. I only noticed that AFTER the PR was merged in :)
Let's make this change here while we're changing things anyway.
lib/helper.ts
Outdated
| * the default variable value will be used instead. | ||
| * @returns {string} The formatted URL with all variable placeholders replaced by values. | ||
| */ | ||
| export function constructServiceURL( |
There was a problem hiding this comment.
BTW, I'm not suggesting we change the Java core :)
This is consistent with the existing `setServiceUrl` method.
## [2.11.1](v2.11.0...v2.11.1) (2021-06-07) ### Bug Fixes * change constructServiceURL from method to function ([#141](#141)) ([cd2d28c](cd2d28c))
|
🎉 This PR is included in version 2.11.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Now it can be used as a utility function instead of an inherited method.
An SDK class can now include a
constructServiceURLmethod with a different signature than this function.Checklist
npm testpasses (tip:npm run lint-fixcan correct most style issues)