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

add replace function #27

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

Conversation

EmileTrotignon
Copy link

Add a replace function to address issue #26

@Drup
Copy link
Owner

Drup commented Nov 9, 2024

Thanks for the patch !

oh, wow, it took me several time to understand what you were going for ... It's impressively elegant in term of implementation, but my gut feeling is that it's a horrible API. This is reinforced by the need for a one-way converter.

Why not something of the form 'a Tyre.t -> ('a -> string) -> string -> (string, _) error instead ? Or even simpler string Tyre.t -> (string -> string) -> string -> (string, _) error

Overall, I agree it's a very interesting to add replace, but I must say it's not clear to me what we gain over just using Re.

@EmileTrotignon
Copy link
Author

It's impressively elegant in term of implementation, but my gut feeling is that it's a horrible API. This is reinforced by the need for a one-way converter.

I guess my idea was to use regexp without having to do anything outside it, but its true that in this case having an 'a -> string parameter is good because you do not need it both ways for replace, which conv requires. It will also not make the implementation very different.

I must say it's not clear to me what we gain over just using Re.

A lot ! Even if the regexp returns a string at the end, does not mean you cannot use the typing in the middle. In the test I wrote for instance, I multiply every int in a string by two. Doing that with Re would require unsafe calls to int_of_string.
Anyway, I want to use Tyre with everything, even for use-cases where Re is just as good, because what if I need to do more complex stuff later and then the typing is critical ?

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