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

RegEx messages #2276

Merged
merged 11 commits into from
Mar 14, 2017
Merged

RegEx messages #2276

merged 11 commits into from
Mar 14, 2017

Conversation

55
Copy link
Contributor

@55 55 commented Mar 10, 2017

Closes #2160

@55 55 added the in progress label Mar 10, 2017
@55 55 added this to the Sprint 38 milestone Mar 10, 2017
{ exp: SimpleSchema.RegEx.Email, msg: '[label] must be a valid e-mail address' },
{ exp: proxyBasePathRegEx, msg: '[label] allowed characters: A-Z' },
{ exp: apiBasePathRegEx, msg: '[label] allowed characters: A-Z' },
{ exp: contactPhone, msg: '[label] allowed characters: A-Z' },
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not correspond with the contactPhone regex.

@brylie brylie self-assigned this Mar 13, 2017
@brylie
Copy link
Contributor

brylie commented Mar 13, 2017

I see this is a work-in-progress, so I will wait till it is closer for comments. It is looking good for a work-in-progress, though.

@@ -0,0 +1,3 @@
// all numbers (0-9) , + , - , space ,/ are allowed and can appear anywhere in the phone numbers
// e.g. 754-3010,(541) 754-3010,1-541-754-3010,1-541-754-3010,191 541 754 3010,(089) / 636-48018
export const contactPhone = new RegExp(/^[0-9-+()/\s.]+$/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use export default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using export default as

const contactPhone = new RegExp(/^[0-9-+()/\s.]+$/);
export default contactPhone

gives me:

Error: Invalid definition for contact.phone field.
W20170313-13:04:17.232(2)? (STDERR)     at packages/aldeed_simple-schema.js:1429:13
W20170313-13:04:17.233(2)? (STDERR)     at Function._.each._.forEach (packages/underscore.js:147:22)
W20170313-13:04:17.233(2)? (STDERR)     at [object Object].SimpleSchema (packages/aldeed_simple-schema.js:1426:5)
W20170313-13:04:17.234(2)? (STDERR)     at meteorInstall.organizations.collection.schema.js (organizations/collection/schema.js:12:24)
W20170313-13:04:17.234(2)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:197:9)
W20170313-13:04:17.235(2)? (STDERR)     at require (packages/modules-runtime.js:120:16)

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, it gave. Use the correct import structure:

import contactPhone from '/path-to-library/';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks. Missed that.

@55
Copy link
Contributor Author

55 commented Mar 13, 2017

@apinf/developers can anybody help me with these regExs:

RegExp(/^\/[\w\-/\.:\?#@!\$&\*\+,;=\'\(\)]+\/$/);
RegExp(/^\/[\w\-/\.:\?#@!\$&\*\+,;=\'\(\)]*$/);

What are allowed characters here?

@brylie
Copy link
Contributor

brylie commented Mar 13, 2017

@apinf/developers can anybody help me with these regExs:

RegExp(/^\/[\w\-/\.:\?#@!\$&\*\+,;=\'\(\)]+\/$/);
RegExp(/^\/[\w\-/\.:\?#@!\$&\*\+,;=\'\(\)]*$/);

Basically, anything with a \ in front of it. I.e. if you remove all, but one, of the \ characters, you will be left with the character list.

@matleppa could you add a comment before those lines of code, to explain them in plain English?

@shaliko
Copy link
Contributor

shaliko commented Mar 13, 2017

@brylie May be will be useful http://scriptular.com/

@brylie
Copy link
Contributor

brylie commented Mar 13, 2017

@shaliko thanks for the link. It seems like @NNN is wanting more of an explanation for what the regex does, as opposed to 'feeling around' to see what patterns it matches. To this end, Regex101 has an explanation tab that describes what a given regex is doing:

screenshot_20170313_134540

@matleppa
Copy link
Member

matleppa commented Mar 13, 2017

[\w\-/\.:\?#@!\$&\*\+,;=\'\(\)]
The characters indicated inside the square brackets are allowed followingly (sets are here separated with |):
\w allows: a-zA-Z0-9_ | following characters are allowed as such: /:#@!&,;= | following characters are allowed (but they must here be escaped with \ ): \- \. \? \$ \* \+ \' \( \) so allowed characters are following: -.?$*+'()

@55
Copy link
Contributor Author

55 commented Mar 13, 2017

Open for suggestion on i18n token names and messages text.

Otherwise, ready for review.

@@ -12,8 +12,8 @@ SimpleSchema.messages({
{ exp: SimpleSchema.RegEx.Id, msg: '[label] must be a valid alphanumeric ID' },
{ exp: SimpleSchema.RegEx.Domain, msg: '[label] must be a valid domain' },
{ exp: SimpleSchema.RegEx.Email, msg: '[label] must be a valid e-mail address' },
{ exp: proxyBasePathRegEx, msg: 'Allowed characters for [label]: A-Z' },
{ exp: apiBasePathRegEx, msg: 'Allowed characters for [label]: A-Z' },
{ exp: proxyBasePathRegEx, msg: '[label] must begin and end with "/"' },
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't recall if it is difficult/impossible to internationalize these regex validation messages. Since they are defined in a common folder, rather than a client folder, we may have encountered difficulties in the past.

@brylie
Copy link
Contributor

brylie commented Mar 14, 2017

@matleppa would you like to finish this PR review?

@matleppa
Copy link
Member

Allowed alphanumeric characters and-.?$*+'()

There should also be mentioned underscore _
Also there should be a space between (and- --> and -)

@matleppa
Copy link
Member

The end of my earlier comment was misleading.
All those mentioned characters are allowed, I just presented in three groups.

-.?$*+'()/:#@!&,;=

@55
Copy link
Contributor Author

55 commented Mar 14, 2017

@matleppa done

@matleppa matleppa merged commit 2c86ffc into develop Mar 14, 2017
@matleppa matleppa deleted the feature/regex-messages branch March 14, 2017 12:46
@brylie
Copy link
Contributor

brylie commented Mar 15, 2017

@matleppa it does not appear that this PR included code comments for the baseUrl regular expression. Please remember to add those comments, as they would clearly be helpful.

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.

None yet

5 participants