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 the check on examples/fillform.js #1022

Merged
merged 2 commits into from
Sep 1, 2023
Merged

Fix the check on examples/fillform.js #1022

merged 2 commits into from
Sep 1, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Aug 31, 2023

What?

This change will fix the check in the example/fillform.js test.

Why?

check actually expects a function that returns a boolean, as is documented. Although the check accepts the object as we've been using it, it's not correct. When users work with this example and the type definitions for k6, they will see a red line under the 'header' in check, which will cause confusion and annoyance as to why our own examples have errors.

Screenshot 2023-08-31 at 14 48 04

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

@ankur22 ankur22 changed the title Fix the check on fillform Fix the check on examples/fillform.js Aug 31, 2023
@ankur22 ankur22 requested review from inancgumus and ka3de August 31, 2023 13:50
@ankur22 ankur22 force-pushed the fix/example-fillform branch from a46b212 to f45856d Compare August 31, 2023 13:58
inancgumus
inancgumus previously approved these changes Sep 1, 2023
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

LGTM! I guess there is a superfluous commit: Add some.

examples/fillform.js Outdated Show resolved Hide resolved
ankur22 added a commit that referenced this pull request Sep 1, 2023
This will help distinguish between page and the local variable in the
check.

Resolves: #1022 (comment)
Co-authored-by: İnanç Gümüş <[email protected]>
@ankur22 ankur22 force-pushed the fix/example-fillform branch from f45856d to 84ca43c Compare September 1, 2023 08:53
inancgumus
inancgumus previously approved these changes Sep 1, 2023
ankur22 added a commit that referenced this pull request Sep 1, 2023
This will help distinguish between page and the local variable in the
check.

Resolves: #1022 (comment)
Co-authored-by: İnanç Gümüş <[email protected]>
@ankur22 ankur22 force-pushed the fix/example-fillform branch from 84ca43c to 27ded62 Compare September 1, 2023 08:58
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for polishing these details.
LGTM.

@ankur22 ankur22 requested a review from inancgumus September 1, 2023 10:10
ankur22 and others added 2 commits September 1, 2023 12:04
The check on the fillform example test worked but is not quite correct.
Check actually expects a function that returns a boolean, as is is
documented.
This will help distinguish between page and the local variable in the
check.

Resolves: #1022 (comment)
Co-authored-by: İnanç Gümüş <[email protected]>
@ankur22 ankur22 force-pushed the fix/example-fillform branch from 27ded62 to 0b5d155 Compare September 1, 2023 11:04
@ankur22 ankur22 merged commit d9bf638 into main Sep 1, 2023
12 checks passed
@ankur22 ankur22 deleted the fix/example-fillform branch September 1, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants