Skip to content

Implementation of white space normalisation.#649

Merged
Marsup merged 1 commit intohapijs:masterfrom
usrz:master
Jun 6, 2015
Merged

Implementation of white space normalisation.#649
Marsup merged 1 commit intohapijs:masterfrom
usrz:master

Conversation

@pfumagalli
Copy link
Contributor

This is an implementation of the request made at #648

I have added the string.normalize() function (basically inspired by trim()) following the name XSLT/XPath/XQuery gave to it (I was in doubt whether to call this normalizeSpace() or normalize_space(), but it seems JOI prefers shorter names).

I'm always baffled by normalise vs. normalize... I guess I'll go with the latter (again, XSLT/XPath/XQuery...)

@AdriVanHoudt
Copy link
Contributor

is there a possibility to broaden this to also include the use case from #586?

@pfumagalli
Copy link
Contributor Author

I don't see why not, assuming that you mean string.replace(...) and not any.transform(function(...) {...})

I can definitely tackle the first case, the latter might be a bit more than I can chew at the moment!

WDYT?

pfumagalli added a commit to usrz/joi that referenced this pull request May 19, 2015
@pfumagalli
Copy link
Contributor Author

I gave a shot to implementing replace(pattern, replacement)... Apparently Travis thinks all is well :-)

test/string.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this extra white line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gone...

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@hueniverse hueniverse added the feature New functionality or improvement label May 21, 2015
@pfumagalli
Copy link
Contributor Author

I was wondering if this could be by any change merged :-)

lib/string.js Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to also be able to provide a string and transform it to a regexp with flag g to replace everything. Don't forget to use Hoek.escapeRegex on the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Marsup Marsup self-assigned this Jun 5, 2015
@Marsup Marsup added this to the 6.5.0 milestone Jun 5, 2015
pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
…g from this rule, so just clone the current object. hapijs#649
@pfumagalli
Copy link
Contributor Author

@Marsup I've tried to address all your concerns/comments in the latest few commits!

Let me know if there's anything else I should fix (at least Travis is happy)!

lib/string.js Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

typeof is not a function, no parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whops :-) I wish there was a esLint def for that! Fixed.

pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
lib/string.js Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still thinking about it, convert is documented as a type coercing property, this is not the case here, I'm not sure we need it. (though we could argue about lowercase and others)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, the string should be left untouched unless the convert flag was specified (at validation time)

True that replace IMPLIES a modification (does not execute any test) but when it is paired with regex, it makes for some interesting constructs. For example:

var schema = joi.string()
                .regex(/^[^x]*$/)
                .replace(/x/, 'y')
                .trim();

function validateOrNormalize(input, loose) {

  var result = joi.validate(input, schema, { convert: false } );
  if (result.error) {
    console.log("WARNING", result.error.details);
    if (loose) {
      return joi.validate(input, schema, { convert: true }).value;
    } else {
      throw result.error;
    }
  } else {
    return  input;
  }
}

Dunno, food for thought...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well then, move it back inside the if. Also can you squash your commits before I merge ?

pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
pfumagalli added a commit to usrz/joi that referenced this pull request Jun 6, 2015
- Provide a string and transform it to a regexp with flag g to replace everything.
- Do not push functions, push parameters that will be used directly in the loop up there without function calls.
- Rebuilt TOC.
- Remove reference to convert in README.
- Document parameters as in most of the other parts.
- Reworked logic as this can not be considere a test like trim.
@Marsup
Copy link
Collaborator

Marsup commented Jun 6, 2015

You should probably rebase it on the current master, there are way too many commits.

@pfumagalli
Copy link
Contributor Author

Yup. Doing that right now....

On Sat, Jun 6, 2015 at 6:21 PM, Nicolas Morel notifications@github.com
wrote:

You should probably rebase it on the current master, there are way too
many commits.


Reply to this email directly or view it on GitHub
#649 (comment).

@pfumagalli
Copy link
Contributor Author

All squashed and Travis is happy, too!

Marsup added a commit that referenced this pull request Jun 6, 2015
Implementation of white space normalisation.
@Marsup Marsup merged commit bb62f65 into hapijs:master Jun 6, 2015
@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.

4 participants