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 glob matching of alternatives #6839

Merged
merged 1 commit into from
Jan 4, 2024
Merged

fix glob matching of alternatives #6839

merged 1 commit into from
Jan 4, 2024

Conversation

sokra
Copy link
Member

@sokra sokra commented Dec 20, 2023

Description

The unicode refactoring introduced some bugs in matching of alternatives

Closes PACK-2172

Copy link

vercel bot commented Dec 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rust-docs ❌ Failed (Inspect) Dec 20, 2023 11:23pm
10 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-gatsby-web ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-native-web ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-nonmonorepo ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-svelte-web ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-tailwind-web ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
examples-vite-web ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm
turbo-site ⬜️ Ignored (Inspect) Dec 20, 2023 11:23pm

};

let c = &chars[self.cursor.cur_cursor()..end]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unused, but it doesn't really make sense to use the path cursor to index chars

}
} else if let Some(alternative) = alternatives.get(self.cursor.cur_cursor()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

path cursor can't be used to index alternatives list

@sokra sokra requested a review from kwonoj December 20, 2023 23:24
Copy link
Contributor

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Comment on lines +272 to +274
let Ok(Some(_)) = self.cursor.next_boundary(self.path, 0) else {
return None;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just shorter and cleaner

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I'm always forgetting there's let..else syntax

Copy link
Contributor

✅ This change can build next-swc

Copy link
Contributor

github-actions bot commented Dec 20, 2023

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

Comment on lines +272 to +274
let Ok(Some(_)) = self.cursor.next_boundary(self.path, 0) else {
return None;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

yes I'm always forgetting there's let..else syntax

#[case::alternatives_nested1("{a,b/c,d/e/{f,g/h}}", "a")]
#[case::alternatives_nested2("{a,b/c,d/e/{f,g/h}}", "b/c")]
#[case::alternatives_nested3("{a,b/c,d/e/{f,g/h}}", "d/e/f")]
#[case::alternatives_nested4("{a,b/c,d/e/{f,g/h}}", "d/e/g/h")]
// #[case::alternatives_chars("[abc]", "b")]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this inteded to be commented?

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't supported in the glob parsing yet. We might want to uncomment that test case once we implement that.

@sokra sokra merged commit 64b2cba into main Jan 4, 2024
49 of 52 checks passed
@sokra sokra deleted the sokra/bugfix-glob-matching branch January 4, 2024 12:57
Zertsov pushed a commit that referenced this pull request Jan 5, 2024
### Description

The unicode refactoring introduced some bugs in matching of alternatives


Closes PACK-2172
sokra added a commit that referenced this pull request Jan 8, 2024
sokra added a commit that referenced this pull request Jan 8, 2024
timneutkens pushed a commit to vercel/next.js that referenced this pull request Jan 10, 2024
* vercel/turborepo#6720 <!-- Tobias Koppers - fix
weird local name, add tests -->
* vercel/turborepo#6832 <!-- Leah -
fix(turbopack-ecmascript): make sure async module wrapper is always
generated -->
* ~vercel/turborepo#6885 <!-- Tobias Koppers - fix
aggregation of outdated children and collectibles -->
* ~vercel/turborepo#6839 <!-- Tobias Koppers - fix
glob matching of alternatives -->~
* vercel/turborepo#6884 <!-- Donny/강동윤 - Update
`swc_core` to `v0.87.16` -->
* vercel/turborepo#6964 <!-- Tobias Koppers - Reduce
calls, tasks and duplicate work -->
sokra added a commit that referenced this pull request Jan 10, 2024
Zertsov pushed a commit that referenced this pull request Jan 10, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

The unicode refactoring introduced some bugs in matching of alternatives


Closes PACK-2172
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

The unicode refactoring introduced some bugs in matching of alternatives


Closes PACK-2172
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

The unicode refactoring introduced some bugs in matching of alternatives


Closes PACK-2172
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

The unicode refactoring introduced some bugs in matching of alternatives


Closes PACK-2172
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
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