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

Split by Delimiters Passed To Shave Function #412

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CJames95
Copy link

@CJames95 CJames95 commented Jun 3, 2024

Proposed Changes

  • Adds support to allow users to pass the delimiters they want to use. This will allow users to determine how the string should be broken up for the purpose of truncating text.

CJames95 added 2 commits June 3, 2024 16:31
Added support for delimiters of varying types.
…-Than-Space

Split by Delimiters Passed To Shave Function
@CJames95
Copy link
Author

CJames95 commented Jun 3, 2024

This pull request is for #411 that I made. This was how I fixed it for my project.

@yowainwright
Copy link
Owner

@CJames95 thanks for the PR! I'll reach out in the issue you created.

@yowainwright
Copy link
Owner

@CJames95 the demo you did was GREAT, and I'd love to get this feature in b/c it's a nice touch!!!

As it stands today, I'd like to make this feature so that the delimiter is drop-in replacement for ' '.

  • no extra iteration (no more while loops) and
  • no regex.

When this product was made, speed was key.

  • My concern with the regex is stack overflow.
  • My concern with the extra while loop is just a little less speed for customers who've been using shave for almost 10 years without the need for a delimiter.

Thank you! Happy to be enlightened on any ideas you have!

@CJames95
Copy link
Author

CJames95 commented Jun 7, 2024

@yowainwright I am not sure I quite understand what you mean by

As it stands today, I'd like to make this feature so that the delimiter is drop-in replacement for ' '.

As far as you're concerns for Regex and any additional loops, that makes complete sense. This demo was a quick fix to a problem my team and I encountered and needed to solve.

Admittedly though, I'm not entirely sure how to accomplish this under the constraints at the moment, but I'll look into this more and see if I can come up with some alternatives.

@yowainwright
Copy link
Owner

yowainwright commented Jun 10, 2024

@CJames95 I can look too! ...Might be a little bit but great work here! THANK YOU!
I just want to ensure this nice feature doesn't affect other users. 🙌

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.

2 participants