Skip to content
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

Add ignoreUndefined flag for rename() #616

Merged
merged 3 commits into from
Mar 26, 2015
Merged

Add ignoreUndefined flag for rename() #616

merged 3 commits into from
Mar 26, 2015

Conversation

JbIPS
Copy link

@JbIPS JbIPS commented Mar 25, 2015

Add a flag to ignore a missing key when renaming it.

var schema = Joi.object().rename('b', 'a', { ignoreUndefined: true });

            var input = {
                a: 'something'
            };

            schema.validate(input, function (err, value) {

               // input : {a: 'something'}
            });

close #614


expect(err).to.not.exist();
expect(Object.keys(value)).to.include('a');
expect(value.a).to.equal('something');
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it easier to do expect(value).to.deep.equal({ a: 'something' })?

Copy link
Author

Choose a reason for hiding this comment

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

That's right, I just copied/paste https://github.com/JbIPS/joi/blob/master/test/object.js#L652.
If you think deep.equal() is better, I'll change it

Copy link
Contributor

Choose a reason for hiding this comment

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

it will make sure that if you tamper with the test and add an extra key or something the test itself will still want to same result. in your case the results needs to have key a but it can aswell have key b. I don't think it is a major thing but it also improves readability imo

Copy link
Author

Choose a reason for hiding this comment

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

Ok, the new test is here :)

@AdriVanHoudt
Copy link
Contributor

👍

@@ -84,6 +84,10 @@ internals.Object.prototype._base = function (value, state, options) {
for (var r = 0, rl = this._inner.renames.length; r < rl; ++r) {
var item = this._inner.renames[r];

if (item.options.ignoreUndefined && target[item.from] === undefined){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Space before brace.

@Marsup
Copy link
Collaborator

Marsup commented Mar 25, 2015

To avoid future regressions can you also add tests combining this flag with override true ? And also add that flag to the docs. Thanks !

@JbIPS
Copy link
Author

JbIPS commented Mar 26, 2015

Added a cross test with override and rebased to enable auto merge :)

@AdriVanHoudt
Copy link
Contributor

nice work!

Marsup added a commit that referenced this pull request Mar 26, 2015
Add ignoreUndefined flag for rename()
@Marsup Marsup merged commit 675af0f into hapijs:6.1.0 Mar 26, 2015
@Marsup
Copy link
Collaborator

Marsup commented Mar 26, 2015

Thanks !

@Marsup Marsup added the feature New functionality or improvement label Mar 26, 2015
@Marsup Marsup added this to the 6.1.0 milestone Mar 26, 2015
@Marsup Marsup self-assigned this Mar 26, 2015
@JbIPS
Copy link
Author

JbIPS commented Mar 26, 2015

Thanks! Just one little question, when do you think this will be released? Just to know when I could go back to "joi": "6.1.0" on my package :)

@Marsup
Copy link
Collaborator

Marsup commented Mar 26, 2015

Finishing the work on IP validation then :shipit:.

@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.

3 participants