Skip to content

Changed a separator for path parameter to pre-split array approach for Joi.reach#1433

Merged
Marsup merged 7 commits intohapijs:masterfrom
ArtemySinitsa:pre-split-array-reach-param
Mar 9, 2018
Merged

Changed a separator for path parameter to pre-split array approach for Joi.reach#1433
Marsup merged 7 commits intohapijs:masterfrom
ArtemySinitsa:pre-split-array-reach-param

Conversation

@ArtemySinitsa
Copy link
Contributor

@ArtemySinitsa ArtemySinitsa commented Feb 12, 2018

Fixed the issue: #1383

This changed the path parameter of Joi.reach from string to array of strings

@Marsup
Copy link
Collaborator

Marsup commented Feb 12, 2018

Thanks, I'd like to also keep the current behavior and not introduce this as a breaking change though. It could likely be done by introducing a local function inside reach and recursively call that instead of this.reach, do you mind doing that ?

@ArtemySinitsa
Copy link
Contributor Author

@Marsup You mean keep current behavior and also allow to pass an array as a parameter or apply array-approach only internally?

@Marsup
Copy link
Collaborator

Marsup commented Feb 12, 2018

Being able to pass either a string or an array, and apply the array logic internally once you've split the string if it's a string.

@ArtemySinitsa
Copy link
Contributor Author

@Marsup Ok, I got, I'll do it

Copy link
Collaborator

@Marsup Marsup left a comment

Choose a reason for hiding this comment

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

Almost there :)

lib/index.js Outdated
for (let i = 0; i < children.length; ++i) {
const child = children[i];
if (child.key === key) {
return this.reach(child.schema, schemaPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can call reach here, no need to go through the assert part again since it's safe.

lib/index.js Outdated
}
};

const schemaPath = typeof path === 'string' ? path.split('.') : path;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you call .slice() in the 2nd case ? I don't want to modify input parameters.

@ArtemySinitsa
Copy link
Contributor Author

@Marsup, thanks for review, I fixed my code according to your suggestions

@Marsup Marsup added this to the 13.2.0 milestone Mar 9, 2018
@Marsup Marsup self-assigned this Mar 9, 2018
@Marsup Marsup added the feature New functionality or improvement label Mar 9, 2018
@Marsup Marsup merged commit 7a1b413 into hapijs:master Mar 9, 2018
@Marsup
Copy link
Collaborator

Marsup commented Mar 9, 2018

Thanks !

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature New functionality or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants