Skip to content

Conversation

@truongminh
Copy link
Contributor

Description

The current rest config key RestBinding.CONFIG in packages/rest/keys.ts is defined as ${CoreBindings.APPLICATION_CONFIG}#rest.

It is safer to use an exported function from context/binding where both the concatenation and split are implemented.

The new key is export const CONFIG = Binding.concatKeyAndPath (CoreBindings.APPLICATION_CONFIG, rest)

@slnode
Copy link

slnode commented Dec 27, 2017

Can one of the admins verify this patch?

* @param path the path
*
*/
static concatKeyAndPath(key: string, path: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to name the method as buildKeyWithPath.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you refactor some tests to use this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

@truongminh Ping. Did you see my comments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my slow response.
I will refactor the code and write some tests for this feature tomorrow.

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM

@raymondfeng raymondfeng merged commit fd804a5 into loopbackio:master Jan 10, 2018
@raymondfeng
Copy link
Contributor

@truongminh Thank you for the contribution!

@renovate renovate bot mentioned this pull request Jun 28, 2022
1 task
@renovate renovate bot mentioned this pull request Jan 22, 2024
1 task
@renovate renovate bot mentioned this pull request Sep 6, 2024
1 task
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.

4 participants