fix: correct flags regex to match server and docs by drazisil · Pull Request #147 · codecov/uploader

drazisil · 2021-06-18T00:02:49Z

fixes #146

Isn't that what `const mask = /^[\w/.,-]+$/` is doing though?

codecov · 2021-06-18T00:23:08Z

Codecov Report

Merging #147 (a5f9338) into master (5856d15) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   91.57%   91.39%   -0.19%     
==========================================
  Files          15       15              
  Lines         546      546              
  Branches      105      105              
==========================================
- Hits          500      499       -1     
  Misses         20       20              
- Partials       26       27       +1

Flag	Coverage Δ
alpine	`91.39% <100.00%> (-0.19%)`	⬇️
linux	`91.39% <100.00%> (-0.19%)`	⬇️
linux-without-git	`∅ <ø> (∅)`
macos	`91.39% <100.00%> (-0.19%)`	⬇️
macos-without-git	`∅ <ø> (∅)`
windows	`90.84% <100.00%> (-0.19%)`	⬇️
windows-without-git	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/helpers/validate.js	`100.00% <100.00%> (ø)`
src/ci_providers/provider_circleci.js	`90.32% <0.00%> (-3.23%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5856d15...a5f9338. Read the comment docs.

drazisil · 2021-06-18T00:38:27Z

I disagree that https://app.codecov.io/gh/codecov/uploader/compare/147/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr%20comments&utm_term=codecov#diff-c3JjL2NpX3Byb3ZpZGVycy9wcm92aWRlcl9jaXJjbGVjaS5qcw== is a coverage drop. 🤷‍♀️

RA80533 · 2021-06-18T10:54:19Z

What is a flag? What should it be validated for? It seems like this is early to validate for anything other than that flags are of type string.

It might be best to use a command-line parsing library for this functionality. Even with the addition of periods to the regular expression, validation would fail on other seemingly “valid” (from the perspective of them being genuine) flags.

drazisil · 2021-06-18T15:01:28Z

What is a flag? What should it be validated for? It seems like this is early to validate for anything other than that flags are of type string.

It might be best to use a command-line parsing library for this functionality. Even with the addition of periods to the regular expression, validation would fail on other seemingly “valid” (from the perspective of them being genuine) flags.

A "flag" in this context is a feature of Codecov https://docs.codecov.com/docs/flags

It's being validated because not all strings are accepted and it's preferred to stop at the upload, rather then at a server error.

RA80533 · 2021-06-18T17:23:50Z

Only lowercase, alphanumeric values are accepted. ^[a-z0-9_\.\-]{1,45}$

Oh, I see. Thanks for the link.

Should the regex be changed to the snippet from the documentation?

https://docs.codecov.com/docs/flags#one-to-one-relationship-of-flags-to-uploads

Correct regex is `^[\w\.\-]+${1,45}`, docs have also been updated.

drazisil · 2021-06-19T04:22:31Z

Should the regex be changed to the snippet from the documentation?

Indeed. Turns out the docs were incorrect as well. Now they both match the server :)

src/helpers/validate.js

drazisil · 2021-06-19T13:19:30Z

Ok, @codecov/open-source . This is ready for review and merge. Finally.

RA80533 · 2021-06-21T09:18:22Z

src/helpers/validate.js

@@ -9,7 +9,7 @@ function validateURL(url) {
 }

 function validateFlags(flags) {


Should this be renamed to validateFlag (singular) since it validates one flag?

Probably , but that's scope creep imo.

RA80533 · 2021-06-21T09:28:03Z

src/helpers/validate.js


 function validateFlags(flags) {
-  const mask = /^(?!-)[\w,-]+$/
+  const mask = /^[\w,\.,\-]{1,45}$/


There is benefit in moving this regex outside.

Currently, every invocation of validateFlags will instantiate a unique regular expression instance which will promptly be destroyed (and never reused) after the function returns. The optimization here is to simply instantiate one regular expression but share it among every invocation of the function. This is common practice with regular expressions in JavaScript because complex patterns can be (relatively) expensive to compile.

Is this intended to match commas?

Suggested change

const mask = /^[\w,\.,\-]{1,45}$/

const mask = /^[\w\.\-]{1,45}$/

fix: do not match commas

Currently, every invocation of validateFlags will instantiate a unique regular expression instance which will promptly be destroyed (and never reused) after the function returns

I don't follow. Except in tests, this regex should generally only be used once per execution

RA80533 · 2021-06-21T09:33:58Z

The boundaries of the regular expression should be tested; namely:

Length: (0, 1, 45, 46)
Characters: (uppercase and lowercase alphabetical; numeral; underline; dash; period; comma)

RA80533 · 2021-06-21T09:38:52Z

Should the regex be changed to the snippet from the documentation?

Indeed. Turns out the docs were incorrect as well. Now they both match the server :)

It appears that the docs are still somewhat incorrect:

^[\w\.\-]+${1,45}$

+$ should be removed, resulting in:

^[\w\.\-]{1,45}$

* chore(deps): update dependency eslint to v7.29.0 (#150) * chore(deps): update alpine docker tag to v3.14.0 (#144) * chore(deps): update dependency jest to v27.0.5 (#156) * fix: correct flags regex to match server and docs (#147) * refactor: all exits moved to cli, throw as needed (#157) * chore: change headers to standard (#158) Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Drazi Crendraven <[email protected]>

test: test for flags with a period

ceee622

drazisil requested a review from a team as a code owner June 18, 2021 00:02

drazisil added 2 commits June 17, 2021 20:05

fix: add period support for flags

6a21a81

fix: correct direction of slash

13b262a

Isn't that what `const mask = /^[\w/.,-]+$/` is doing though?

drazisil marked this pull request as draft June 18, 2021 00:10

fix: place period in correct location

bdc6111

drazisil marked this pull request as ready for review June 18, 2021 00:12

drazisil changed the title ~~Flags with periods~~ Support periods in flags Jun 18, 2021

drazisil mentioned this pull request Jun 18, 2021

ci: switch to new codecov uploader drazisil/sigdb#1

Merged

drazisil added 4 commits June 18, 2021 23:46

fix: adjust flag regex to match docs

e3f92ae

https://docs.codecov.com/docs/flags#one-to-one-relationship-of-flags-to-uploads

test: flags reqex allows flags to start with dash

6d7227d

ci: test server accepts flags starting with dash

c7b72f8

fix: corrected flag regex

2e96fd7

Correct regex is `^[\w\.\-]+${1,45}`, docs have also been updated.

fix: move end symbol to end

8eb4115

drazisil changed the title ~~Support periods in flags~~ fix: correct flags regex to match server and docs Jun 19, 2021

fix: allow options

3704388

drazisil commented Jun 19, 2021

View reviewed changes

src/helpers/validate.js Outdated Show resolved Hide resolved

fix: use range, not plus

63e8d61

RA80533 reviewed Jun 21, 2021

View reviewed changes

fix: do not match commas

a5f9338

thomasrockhu approved these changes Jun 22, 2021

View reviewed changes

drazisil-codecov merged commit 6d2a1ac into codecov:master Jun 22, 2021

drazisil deleted the patch-1 branch June 25, 2021 14:48

		@@ -9,7 +9,7 @@ function validateURL(url) {
		}

		function validateFlags(flags) {

	const mask = /^[\w,\.,\-]{1,45}$/
	const mask = /^[\w\.\-]{1,45}$/

Conversation

drazisil commented Jun 18, 2021

Uh oh!

codecov bot commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

drazisil commented Jun 18, 2021

Uh oh!

RA80533 commented Jun 18, 2021

Uh oh!

drazisil commented Jun 18, 2021

Uh oh!

RA80533 commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drazisil commented Jun 19, 2021

Uh oh!

Uh oh!

drazisil commented Jun 19, 2021

Uh oh!

RA80533 Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

drazisil-codecov Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

RA80533 Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RA80533 Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

drazisil-codecov Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

drazisil-codecov Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

RA80533 commented Jun 21, 2021

Uh oh!

RA80533 commented Jun 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jun 18, 2021 •

edited

Loading

RA80533 commented Jun 18, 2021 •

edited

Loading

RA80533 Jun 21, 2021 •

edited

Loading