fix: correct flags regex to match server and docs #147

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

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

 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 <bot@renovateapp.com> Co-authored-by: Drazi Crendraven <drazisil@users.noreply.github.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correct flags regex to match server and docs #147

fix: correct flags regex to match server and docs #147

drazisil commented Jun 18, 2021

codecov bot commented Jun 18, 2021 •

edited

Loading

drazisil commented Jun 18, 2021

RA80533 commented Jun 18, 2021

drazisil commented Jun 18, 2021

RA80533 commented Jun 18, 2021 •

edited

Loading

drazisil commented Jun 19, 2021

drazisil commented Jun 19, 2021

RA80533 Jun 21, 2021

drazisil-codecov Jun 21, 2021

RA80533 Jun 21, 2021 •

edited

Loading

RA80533 Jun 21, 2021

drazisil-codecov Jun 21, 2021

drazisil-codecov Jun 21, 2021

RA80533 commented Jun 21, 2021

RA80533 commented Jun 21, 2021

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

		function validateFlags(flags) {

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

fix: correct flags regex to match server and docs #147

fix: correct flags regex to match server and docs #147

Conversation

drazisil commented Jun 18, 2021

codecov bot commented Jun 18, 2021 • edited Loading

Codecov Report

drazisil commented Jun 18, 2021

RA80533 commented Jun 18, 2021

drazisil commented Jun 18, 2021

RA80533 commented Jun 18, 2021 • edited Loading

drazisil commented Jun 19, 2021

drazisil commented Jun 19, 2021

RA80533 Jun 21, 2021

Choose a reason for hiding this comment

drazisil-codecov Jun 21, 2021

Choose a reason for hiding this comment

RA80533 Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

RA80533 Jun 21, 2021

Choose a reason for hiding this comment

drazisil-codecov Jun 21, 2021

Choose a reason for hiding this comment

drazisil-codecov Jun 21, 2021

Choose a reason for hiding this comment

RA80533 commented Jun 21, 2021

RA80533 commented Jun 21, 2021

codecov bot commented Jun 18, 2021 •

edited

Loading

RA80533 commented Jun 18, 2021 •

edited

Loading

RA80533 Jun 21, 2021 •

edited

Loading