Skip to content

feat: support flags for hybrid regex#1915

Merged
IWANABETHATGUY merged 5 commits intorolldown:mainfrom
shulaoda:feat/support-flags-for-hybrid-regex
Aug 9, 2024
Merged

feat: support flags for hybrid regex#1915
IWANABETHATGUY merged 5 commits intorolldown:mainfrom
shulaoda:feat/support-flags-for-hybrid-regex

Conversation

@shulaoda
Copy link
Copy Markdown
Member

@shulaoda shulaoda commented Aug 9, 2024

Description

closes #1911

@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 9, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit b408478
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66b5fde3686d99000817d12a

@shulaoda shulaoda marked this pull request as draft August 9, 2024 07:28
@shulaoda shulaoda marked this pull request as ready for review August 9, 2024 07:31
Comment thread crates/rolldown_common/src/types/js_regex.rs Outdated
@IWANABETHATGUY
Copy link
Copy Markdown
Member

The impl is too complex, just pass flag str to regress::Regex::with_flags is enough, the error is expected, if user pass some flag regress don't support

@IWANABETHATGUY
Copy link
Copy Markdown
Member

What you only need is handle flags for regex https://docs.rs/regex/latest/regex/#grouping-and-flags

@shulaoda
Copy link
Copy Markdown
Member Author

shulaoda commented Aug 9, 2024

What you only need is handle flags for regex https://docs.rs/regex/latest/regex/#grouping-and-flags

Should I verify the repeatability of flags for regex or let regex handle it?

@IWANABETHATGUY
Copy link
Copy Markdown
Member

What you only need is handle flags for regex https://docs.rs/regex/latest/regex/#grouping-and-flags

Should I verify the repeatability of flags for regex or let regex handle it?

No, just let lib handle it, the order does not matter.

@shulaoda
Copy link
Copy Markdown
Member Author

shulaoda commented Aug 9, 2024

In addition, I changed the confusing name source to pattern.

Comment thread crates/rolldown_common/src/types/js_regex.rs Outdated
@IWANABETHATGUY
Copy link
Copy Markdown
Member

IWANABETHATGUY commented Aug 9, 2024

Also could you add a basic test to ensure regression?

mod test {
  #[test]
  fn with_flags() {
    let reg = super::HybridRegex::with_flags("a", "i").unwrap();
    assert!(reg.matches("A"));

    let reg = super::HybridRegex::new("a").unwrap();
    assert!(!reg.matches("A"));
  }
}

Copy link
Copy Markdown
Member

@IWANABETHATGUY IWANABETHATGUY left a comment

Choose a reason for hiding this comment

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

Thanks

@IWANABETHATGUY IWANABETHATGUY added this pull request to the merge queue Aug 9, 2024
Merged via the queue into rolldown:main with commit 5ad5820 Aug 9, 2024
@shulaoda shulaoda deleted the feat/support-flags-for-hybrid-regex branch August 9, 2024 11:55
github-merge-queue Bot pushed a commit that referenced this pull request Aug 13, 2024
<!-- Thank you for contributing! -->

### Description

Related to #1914 and #1915 

https://github.com/rolldown/rolldown/blob/6389d62f876a122df451d3e0151f79f59dd978cb/crates/rolldown_plugin_transform/src/lib.rs#L20-L25
<!-- Please insert your description here and provide especially info
about the "what" this PR is solving -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Should support flag in HybridRegex

3 participants