-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX beta] prevent test error by converting illegal characters #16200
[BUGFIX beta] prevent test error by converting illegal characters #16200
Conversation
2fb2e17
to
2e3257b
Compare
broccoli/version.js
Outdated
let sha = info.sha || ''; | ||
// * remove illegal non-alphanumeric characters from branch name. | ||
if(info.branch !== null && new RegExp(/[^a-zA-Z\d\s-]/).test(info.branch)) { | ||
info.branch = info.branch.replace(/[^a-zA-Z\d\s-]/, '-'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind tweaking this a tad bit, I'd like to avoid mutating the info
object here and avoid repeating the regexp.
I think this might do the trick:
// ...snip...
let sha = info.sha || '';
let suffix = process.env.BUILD_TYPE || info.branch;
suffix = suffix.replace(/[^a-zA-Z\d\s-]/, '-');
// ...snip...
@Parrryy - What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwjblue What about the case when the suffix is process.env.BUILD_TYPE
? I'm not exactly sure what that is and if the .replace
will effect it.
c9f615f
to
925d56c
Compare
broccoli/version.js
Outdated
let sha = info.sha || ''; | ||
let suffix = process.env.BUILD_TYPE || info.branch; | ||
// * remove illegal non-alphanumeric characters from branch name. | ||
suffix = suffix.replace(/[^a-zA-Z\d\s-]/, '-'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwjblue The issue now is one I was having before where the suffix
var is null. It passes tests locally, just on Travis it fails. Any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see.
Something like:
suffix = suffix && suffix.replace(...)
Should work I believe...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwjblue Yup, all good now.
…branch name null fix
66f9288
to
d1bc83d
Compare
Awesome, thank you! |
Addresses #16198. Converts all non-alphanumeric characters to '-' to prevent error when running test's.