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

Fix multiple byte tests #31

Merged
merged 5 commits into from
Aug 14, 2021
Merged

Fix multiple byte tests #31

merged 5 commits into from
Aug 14, 2021

Conversation

seriousme
Copy link
Contributor

Byte is defined in the openapi specification as Base64
Base64 data can span multiple lines, hence the /gm.
The challenge with /gm is that it won't reset the lastIndex of the RegExp.
So when the next call tries to use the RegExp it will fail because lastIndex has not been reset.

This PR fixes that by defining the RegExp locally.
Added a test that proves it works ok now, that same test failed without the fix.

Kind regards,
Hans

@seriousme
Copy link
Contributor Author

Had to upgrade prettier to make CI work again ;-)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 836689211

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 92.704%

Files with Coverage Reduction New Missed Lines %
dist/formats.js 1 89.13%
Totals Coverage Status
Change from base Build 811386298: 0.06%
Covered Lines: 128
Relevant Lines: 135

💛 - Coveralls

@coveralls
Copy link

coveralls commented May 12, 2021

Pull Request Test Coverage Report for Build 1130221971

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.09%) to 92.735%

Files with Coverage Reduction New Missed Lines %
dist/formats.js 1 89.21%
Totals Coverage Status
Change from base Build 1130183033: 0.09%
Covered Lines: 129
Relevant Lines: 136

💛 - Coveralls

@seriousme
Copy link
Contributor Author

@epoberezkin could you please review ?

Kind regards,
Hans

@epoberezkin
Copy link
Member

Had to upgrade prettier to make CI work again ;-)

prettier indeed made some change that changed the required formatting - annoying, I've been bitten by it too.

There are no changes in regexes, they are just on the next line, correct?

The challenge with /gm is that it won't reset the lastIndex of the RegExp.

Some quirk... Maybe some other method should be used, or is it just how it is?

@seriousme
Copy link
Contributor Author

seriousme commented May 14, 2021

Had to upgrade prettier to make CI work again ;-)

prettier indeed made some change that changed the required formatting - annoying, I've been bitten by it too.

There are no changes in regexes, they are just on the next line, correct?

Correct, all other changes apart from byte are performed by prettier and just put them on the next line. I haven't touched them.

The challenge with /gm is that it won't reset the lastIndex of the RegExp.

Some quirk... Maybe some other method should be used, or is it just how it is?

It is the recommended way according to StackOverflow ;-)

Kind regards,
Hans

@seriousme
Copy link
Contributor Author

@epoberezkin are you ok with this fix ?

@seriousme
Copy link
Contributor Author

@epoberezkin can you please review again ?

Kind regards,
Hans

Copy link
Member

@epoberezkin epoberezkin left a comment

Choose a reason for hiding this comment

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

Also, to reduce noise, I suggest fixing prettier version at 2.0.5 and I would commit style changes separately

function byte(str: string): boolean {
const BYTE = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/gm
return BYTE.test(str)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think rather than defining regex locally it would be better to reset the index on regex after the call and have regex itself defined once in the scope of this function.

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 keep the code concise I propose to do it like this:

const BYTE = /^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/gm

function byte(str: string): boolean {
  BYTE.lastIndex = 0
  return BYTE.test(str)
}

Ok ?

Copy link
Member

Choose a reason for hiding this comment

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

yes - thank you

@seriousme
Copy link
Contributor Author

I tried downgrading my local prettier to 2.0.5 but that results in other problems:

> prettier --list-different "./**/*.{md,json,yaml,js,ts}"

src/index.ts[error] src/index.ts: SyntaxError: Expression expected. (55:27)
[error]   53 | 
[error]   54 | function addFormats(ajv: Ajv, list: FormatName[], fs: DefinedFormats, exportName: Name): void {
[error] > 55 |   ajv.opts.code.formats ??= _`require("ajv-formats/dist/formats").${exportName}`
[error]      |                           ^
[error]   56 |   for (const f of list) ajv.addFormat(f, fs[f])
[error]   57 | }
[error]   58 | 

So can I propose that you upgrade prettier on master after which I will add my few lines of code?

Kind regards,
Hans

@epoberezkin
Copy link
Member

So can I propose that you upgrade prettier on master after which I will add my few lines of code?

yes - thank you - done!

@epoberezkin
Copy link
Member

@seriousme actually, the diff is now smaller, so the only thing that's left is taking regex out of the function - will you do it?

Thank you

@seriousme
Copy link
Contributor Author

@seriousme actually, the diff is now smaller, so the only thing that's left is taking regex out of the function - will you do it?

Thank you

Like I said, most of the changes were due to prettier ;-)
I have moved the regexp out of the function.

Please review again.

Kind regards,
Hans

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants