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

Check if parameter is number before Number.parse #191

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

zabil
Copy link
Member

@zabil zabil commented Oct 10, 2024

Fixes #190 #187

@zabil zabil requested a review from BugDiver October 10, 2024 14:14
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
@zabil
Copy link
Member Author

zabil commented Oct 10, 2024

@BugDiver not sure if this is the best way to fix it. Do take a look.

const num = Number.parseFloat(value);
return Number.isNaN(num) ? undefined : num;
const trimmedValue = value.trim();
if (/^-?\d+(\.\d+)?$/.test(trimmedValue)) {
Copy link

@anthony-legay anthony-legay Oct 10, 2024

Choose a reason for hiding this comment

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

I thought of using a regex but wanted to avoid it because it seemed overkill, but I'm afraid we don't have a choice.
I came up during my investigations with this regex: /^[+-]?(\d+(\.\d*)?|\.\d+)([eE][+-]?\d+)?$/ (thanks ChatGPT) to handle extreme case where users would use exponential notations (e.g. 2e-32). Seems unlikely, but...well better safe than sorry 😄

What if Gauge allowed the "special parameter" notation to specify a plain string like <string:0123456> and then it would bypass this? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

What if Gauge allowed the "special parameter" notation to specify a plain string like string:0123456 and then it would bypass this?

Thanks for the suggestion! The idea of using a special parameter like <string:0123456> makes sense. However, it's best to keep Gauge specs as readable and close to markdown as possible. This is to have that clear separation and make the reports readable to people who are not that technical.

Copy link
Member Author

Choose a reason for hiding this comment

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

@anthony-legay pushed a simpler approach refer 910e614

Is there a way you can test this locally?

Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
BugDiver
BugDiver previously approved these changes Oct 11, 2024
Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson chadlwilson added the ReleaseCandidate If a PR is tagged with this label, after merging it will be released label Oct 11, 2024
@gaugebot
Copy link

gaugebot bot commented Oct 11, 2024

@zabil Thank you for contributing to gauge-ts. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@chadlwilson
Copy link
Contributor

Fixed the artifact upload/download. Otherwise LGTM

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@zabil
Copy link
Member Author

zabil commented Oct 11, 2024

Fixed the artifact upload/download.

Thanks! @chadlwilson

@zabil zabil merged commit bc7a9be into main Oct 11, 2024
17 checks passed
@anthony-legay
Copy link

Sorry, I'm coming after the party, but this does not actually fix the issue when a value like "0123456" (such as a product id, or serial number) is used as a param and we want it to remains a string.
But this may be part of another bigger issue, what if some kind of serial number (e.g. not an EAN13 but even bigger) is used ? it does not make sens to convert this to a number, and it could even lead to overflow issues. 🫠
I don't know, not blaming anything here, thanks for the fix anyway ! I'm just throwing ideas and warnings about automatic number conversion 🙂

@chadlwilson chadlwilson deleted the fix-paramater-conversion branch October 11, 2024 10:27
@zabil
Copy link
Member Author

zabil commented Oct 11, 2024

when a value like "0123456" (such as a product id, or serial number) is used as a param and we want it to remains a string.

Ah shoot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReleaseCandidate If a PR is tagged with this label, after merging it will be released
Development

Successfully merging this pull request may close these issues.

String parameter beginning with numbers are wrongly interpreted as numbers
4 participants