-
-
Notifications
You must be signed in to change notification settings - Fork 902
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
infra(unicorn): better-regex #2487
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2487 +/- ##
==========================================
- Coverage 99.58% 99.58% -0.01%
==========================================
Files 2787 2787
Lines 249376 249377 +1
Branches 1084 1084
==========================================
- Hits 248333 248330 -3
- Misses 1015 1019 +4
Partials 28 28
|
@@ -297,7 +297,7 @@ describe('internet', () => { | |||
const [prefix, suffix] = email.split('@'); | |||
|
|||
expect(prefix).toMatch( | |||
/^Mike((\d{1,2})|([.!#$%&'*+-/=?^_`{|}~]Smith\d{1,2})|([.!#$%&'*+-/=?^_`{|}~]Smith))/ | |||
/^Mike((\d{1,2})|([!#$%&'*+-/=?^_`{|}~]Smith\d{1,2})|([!#$%&'*+-/=?^_`{|}~]Smith))/ |
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.
For those of you that wonder like me where the .
went:
+-/
=== +
- /
=== +
, -
, .
, /
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.
that is SUPER confusing. A human reading this would never think that
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.
TBH, this regex pattern in general is super confusing. I just looked at it and noped out immediately.
@@ -98,21 +98,21 @@ describe('color', () => { | |||
describe(`rgb({ format: 'css' })`, () => { | |||
it('should return a random rgb color in css format', () => { | |||
const color = faker.color.rgb({ format: 'css' }); | |||
expect(color).match(/^(rgb\([0-9]{1,3}, [0-9]{1,3}, [0-9]{1,3}\))$/); | |||
expect(color).match(/^rgb\((?:\d{1,3}, ){2}\d{1,3}\)$/); |
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.
this seems harder to read than the original
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.
That means that the regex in general is too complex for that use case IMO. Splitting this one statement into multiple expect statements would be welcome:
const color = faker.color.rgb({ format: 'css' });
expect(color.startsWith('rgb')).toBe(true);
expect(color).toContain('(');
expect(color.endsWith(')')).toBe(true);
const values = color.substring(4, color.length - 1).split(',');
expect(values).toHaveLength(3);
for(const value of values) {
const numeric = Number.parseInt(value, 10);
expect(numeric).not.toBe(NaN);
expect(numeric).toBeGreaterThanOrEqual(0);
expect(numeric).toBeLessThanOrEqual(255);
}
}); | ||
}); | ||
|
||
describe(`rgb({ format: 'binary' })`, () => { | ||
it('should return a random rgb color in binary format', () => { | ||
const color = faker.color.rgb({ format: 'binary' }); | ||
expect(color).match(/^([01]{8} [01]{8} [01]{8})$/); | ||
expect(color).match(/^(?:[01]{8} ){2}[01]{8}$/); |
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.
this seems harder to read than the original
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.
In general these are improvements but sometimes it seems to be harder to read.
Team Decision
|
Ref: #2439
Enables the
unicorn/better-regex
lint rule.