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

feat(regular_expression): implement Display for RegularExpression type. #5304

Conversation

rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Aug 28, 2024

Part of #5060

@github-actions github-actions bot added the A-parser Area - Parser label Aug 28, 2024
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from 1547b64 to fbfa407 Compare August 28, 2024 15:30
Copy link

codspeed-hq bot commented Aug 28, 2024

CodSpeed Performance Report

Merging #5304 will not alter performance

Comparing 08-28-feat_regular_expression_implement_display_for_regularexpression_type (1d0bc62) with main (a1523c6)

Summary

✅ 28 untouched benchmarks

@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch 3 times, most recently from 1a6a980 to bdcbebc Compare August 28, 2024 19:42
@rzvxa rzvxa marked this pull request as draft August 29, 2024 07:38
@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch 2 times, most recently from daa5001 to 9c5363f Compare August 29, 2024 08:42
@rzvxa
Copy link
Contributor Author

rzvxa commented Aug 29, 2024

@leaysgur A quick question. Shouldn't Character::value be u16? Isn't it always utf-16?

@rzvxa
Copy link
Contributor Author

rzvxa commented Aug 29, 2024

Sorry, I totally missed the Unicode mode.

@leaysgur
Copy link
Contributor

Yes, all for Unicode mode.
(Feel free to ask anything!)

@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from 1e1e0e6 to 7367de6 Compare August 29, 2024 16:30
@rzvxa rzvxa marked this pull request as ready for review August 29, 2024 16:41
@rzvxa rzvxa requested a review from leaysgur August 29, 2024 16:41
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Aug 30, 2024 — with Graphite App
@overlookmotel
Copy link
Contributor

overlookmotel commented Aug 30, 2024

@rzvxa Boshen has added the merge label but I'm unclear if this is ready for merge or not. Not sure if the open comment threads need resolution or not.

@Boshen Boshen removed the 0-merge Merge with Graphite Merge Queue label Aug 31, 2024
@Boshen
Copy link
Member

Boshen commented Aug 31, 2024

@Boshen Boshen added the merge label Aug 30, 2024 — with Graphite App

graphite acting on my behalf ... wtf

@Boshen
Copy link
Member

Boshen commented Sep 1, 2024

@leaysgur feel free to add the merge label for graphite to merge.

@leaysgur
Copy link
Contributor

leaysgur commented Sep 1, 2024

@leaysgur feel free to add the merge label for graphite to merge.

🫡

@rzvxa Please let me know when you're ready for the review~!

@overlookmotel overlookmotel added the 0-merge Merge with Graphite Merge Queue label Sep 2, 2024 — with Graphite App
@overlookmotel overlookmotel removed the 0-merge Merge with Graphite Merge Queue label Sep 2, 2024
@rzvxa rzvxa requested a review from leaysgur September 2, 2024 17:57
@rzvxa
Copy link
Contributor Author

rzvxa commented Sep 2, 2024

@leaysgur feel free to add the merge label for graphite to merge.

🫡

@rzvxa Please let me know when you're ready for the review~!

It should be ready for review, Let me know if there are any other issues.

@rzvxa rzvxa marked this pull request as draft September 2, 2024 19:51
@rzvxa
Copy link
Contributor Author

rzvxa commented Sep 2, 2024

@leaysgur Sorry I marked it as draft again, Found a panic while running conformance tests on #5256

@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from b92d023 to 7c5d4b2 Compare September 2, 2024 23:01
@rzvxa rzvxa marked this pull request as ready for review September 2, 2024 23:02
@rzvxa
Copy link
Contributor Author

rzvxa commented Sep 2, 2024

@leaysgur OK it is ready, Sorry for the inconvenience.

Copy link
Contributor

@leaysgur leaysgur left a comment

Choose a reason for hiding this comment

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

👀

crates/oxc_regular_expression/src/display.rs Outdated Show resolved Hide resolved
crates/oxc_regular_expression/src/display.rs Outdated Show resolved Hide resolved
crates/oxc_regular_expression/src/display.rs Outdated Show resolved Hide resolved
crates/oxc_regular_expression/src/display.rs Outdated Show resolved Hide resolved
@rzvxa
Copy link
Contributor Author

rzvxa commented Sep 3, 2024

@leaysgur Made the requested changes, Please give it another go.

Copy link
Contributor

@leaysgur leaysgur left a comment

Choose a reason for hiding this comment

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

👍🏻👍🏻

@leaysgur leaysgur added the 0-merge Merge with Graphite Merge Queue label Sep 3, 2024
Copy link

graphite-app bot commented Sep 3, 2024

Merge activity

  • Sep 2, 9:56 PM EDT: @leaysgur we removed the merge queue label because we could not find a Graphite account associated with your GitHub profile.

You must have a Graphite account in order to use the merge queue. Create an account and try again using this link

@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Sep 3, 2024
@leaysgur
Copy link
Contributor

leaysgur commented Sep 3, 2024

Sorry, I've accidentally put a merge label on it!
And CI seems to be fail now, what happens in this case...?

@rzvxa rzvxa force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch 2 times, most recently from 3aaf2bf to a5ba8b5 Compare September 3, 2024 02:15
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Sep 3, 2024 — with Graphite App
@Boshen Boshen force-pushed the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch from a5ba8b5 to c0b6269 Compare September 3, 2024 02:20
@graphite-app graphite-app bot merged commit c0b6269 into main Sep 3, 2024
21 of 23 checks passed
@graphite-app graphite-app bot deleted the 08-28-feat_regular_expression_implement_display_for_regularexpression_type branch September 3, 2024 02:28
@Boshen
Copy link
Member

Boshen commented Sep 3, 2024

Sorry, I've accidentally put a merge label on it! And CI seems to be fail now, what happens in this case...?

I added the merge label on the next PR. #5278

But CI passed?

@Boshen
Copy link
Member

Boshen commented Sep 3, 2024

@leaysgur FYI it won't get merged if CI fails even if we added the merge label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0-merge Merge with Graphite Merge Queue A-parser Area - Parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants