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

Use faster-than-regex .substring() slicing #84

Merged
merged 2 commits into from
Oct 3, 2021
Merged

Use faster-than-regex .substring() slicing #84

merged 2 commits into from
Oct 3, 2021

Conversation

jorgebucaran
Copy link
Owner

This improves runtime performance when clearing bleeding sequences, using .substring() rather than regexes.

This clever idea is inspired by @alexeyraspopov's replaceClose function in picocolors. Thank you for that! 🙌

Improve runtime performance when clearing bleeding sequences.
@codecov-commenter
Copy link

Codecov Report

Merging #84 (57d4fad) into main (eba26cd) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #84   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          133       147   +14     
=========================================
+ Hits           133       147   +14     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eba26cd...57d4fad. Read the comment docs.

@ai
Copy link
Contributor

ai commented Oct 2, 2021

Add mentions about inspiration to docs?

@kibertoad
Copy link
Collaborator

@jorgebucaran I think that's a good idea. Let's add a section dedicated to credits and honourable mentions.

@jorgebucaran jorgebucaran added the enhancement New feature or request label Oct 3, 2021
@jorgebucaran jorgebucaran merged commit 0233765 into main Oct 3, 2021
@jorgebucaran jorgebucaran deleted the substring branch October 3, 2021 03:16
jorgebucaran added a commit that referenced this pull request Oct 3, 2021
@jorgebucaran
Copy link
Owner Author

That's a great idea. Done in 6a4a871! 💯

@ai
Copy link
Contributor

ai commented Oct 3, 2021

@jorgebucaran 6a4a871 will be better by adding explicit link to chalk and picocolors’s mention and link.

@jorgebucaran
Copy link
Owner Author

I'm pretty happy with the result, actually, but I'll definitely consider your suggestion. Thanks!

@alexeyraspopov
Copy link

The acknowledgment sounds like it was my contribution to this project, which it wasn’t.

I don’t mind adopting performance tricks.

I would really appreciate if you could update this section with proper links to the projects. Would be even better if the section wasn’t buried somewhere down the page. Thanks

@jorgebucaran
Copy link
Owner Author

Sure, I can think of another way to acknowledge you. 👌

@kibertoad
Copy link
Collaborator

@alexeyraspopov We ended up listing reference in release notes next to a specific change, which seems more appropriate, since this is a one-off optimization and not something fundamental to how Colorette is built.
Agreed on readme notes making it sound like it was contributed by you directly. Would you prefer us rephrasing, or removing it and just keeping release notes?

@alexeyraspopov
Copy link

I’d prefer adding links to the projects, as I mentioned previously.

@kibertoad
Copy link
Collaborator

Yup, we are adding links for sure, give us a bit of time :)

@jorgebucaran
Copy link
Owner Author

Absolutely. Added in ada9fb3. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants