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

Support RegExp v flag #2195

Merged
merged 7 commits into from
Sep 25, 2023
Merged

Support RegExp v flag #2195

merged 7 commits into from
Sep 25, 2023

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Sep 22, 2023

No description provided.

@fisker

This comment was marked as outdated.

@fisker
Copy link
Collaborator Author

fisker commented Sep 23, 2023

@novemberborn Sorry for bothering you.

As you can see test is failing in this PR. We use a old version of ava

"ava": "^3.15.0",
, we use it weird, we delete snapshots, and force ava to regenerate snapshots, to make sure snapshots won't change.
- run: rm -rf test/snapshots
# Force update snapshots, https://github.com/avajs/ava/discussions/2754
- run: npx c8 ava --update-snapshots
env:
AVA_FORCE_CI: not-ci
- run: git diff --exit-code

It works well until this PR. I already regenerate snapshots locally, but it still changes on CI. Maybe you known what possibly go wrong?

@novemberborn
Copy link

Looks like a test failed, it didn’t get to the diffing of the snapshots. And only in Node 20. Wouldn’t that be a real bug?

we use it weird, we delete snapshots, and force ava to regenerate snapshots, to make sure snapshots won't change

That sounds like what AVA checks for in CI, what am I missing? Is this to handle differences between Node versions?

@fisker
Copy link
Collaborator Author

fisker commented Sep 24, 2023

Looks like a test failed, it didn’t get to the diffing of the snapshots.

You are right.. I thought that's the "lint-test" job, which does the snapshot diff check, but it's not, really sorry.

Wouldn’t that be a real bug?

Run test on Node.js v20 locally, tests all passed.
Run out of ideas...

@fisker
Copy link
Collaborator Author

fisker commented Sep 24, 2023

Should be something wrong here

const isRegExpWithoutGlobalFlag = (node, scope) => {
const staticResult = getStaticValue(node, scope);
// Don't know if there is `g` flag
if (!staticResult) {
return false;
}
const {value} = staticResult;
return (
Object.prototype.toString.call(value) === '[object RegExp]'
&& !value.global
);
};

I'll try to fix it later.

@fisker
Copy link
Collaborator Author

fisker commented Sep 24, 2023

@novemberborn You are right, there is a bug indeed, thanks for your help.

@fisker fisker marked this pull request as ready for review September 24, 2023 16:21
@sindresorhus sindresorhus merged commit 28e7498 into sindresorhus:main Sep 25, 2023
21 checks passed
@fisker fisker deleted the v-flg branch September 25, 2023 06:06
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