Implement `Str.dropPrefix` and `Str.dropSuffix` builtins

roc-lang / roc

A fast, friendly, functional language.

https://roc-lang.org

Universal Permissive License v1.0

4.46k stars 313 forks source link

Implement `Str.dropPrefix` and `Str.dropSuffix` builtins #7007

Closed lukewilliamboswell closed 1 month ago

lukewilliamboswell commented 3 months ago

Implements the builtins from https://github.com/roc-lang/roc/issues/664

Note I changed the api from what was described in the issue. This was easier to implement and test, and I think will be nicer to use.

dropPrefix : Str, Str -> Result Str {} to dropPrefix : Str, Str -> Str
dropSuffix : Str, Str -> Result Str {} to dropSuffix : Str, Str -> Str

Also, I am not 100% around the use of substringUnsafe here. I think it's ok because we are starting with a valid utf8 string, and using a valid utf8 string to drop so I don't think there is anyway to drop and now have invalid utf8. Is there a way to test this? maybe we should do some fuzzing here?

lukewilliamboswell commented 3 months ago

Useful information Re utf-8 validation https://roc.zulipchat.com/#narrow/stream/304641-ideas/topic/builtins.20for.20parsing/near/463098204

Thank you @timotree3

timotree3 commented 3 months ago

Also, I am not 100% around the use of substringUnsafe here. I think it's ok because we are starting with a valid utf8 string, and using a valid utf8 string to drop so I don't think there is anyway to drop and now have invalid utf8. Is there a way to test this? maybe we should do some fuzzing here?

I agree with your reasoning as long as Str.startsWith haystack prefix == Bool.true guarantees that haystack starts with the same bytes as prefix. If it was weakened to something like "haystack starts with characters that are the same as prefix after some kind of normalization" then this implementation would become buggy.

github-actions[bot] commented 2 months ago

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!