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 library crate #195

Closed
wants to merge 4 commits into from
Closed

Conversation

PeterlitsZo
Copy link

This PR is based on #193. So do not merge me before when #193 is merged, please.

This PR is going to solve #194. It supports users can:

sd::replace("foo", "bar", sd::ReplaceConf {
    source: sd::Source::with_files(vec!["./foo.md", "./bar.md"]),
    ..sd::ReplaceConf::default(),
})

Maybe using a builder is a great idea? And I also need more tests for it.

Your advice is important to me, @CosmicHorrorDev and @fujidaiti. What do you think?

src/lib.rs Outdated
/// ..ReplaceConf::default()
/// })
/// ```
pub fn replace(find: String, replace_with: String, replace_conf: ReplaceConf) -> Result<()> {
Copy link

@fujidaiti fujidaiti Jun 1, 2023

Choose a reason for hiding this comment

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

Looks good, thank you! One thing I'm wondering, though, is it possible to use &str instead of String as the type for find and replace_with? Otherwise, we would always have to create and move Strings. In the current implementation, for example, replace("foo", "bar", ...) is not possible.

Copy link
Author

Choose a reason for hiding this comment

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

Of course.

@PeterlitsZo PeterlitsZo marked this pull request as draft June 1, 2023 16:08
@CosmicHorrorDev
Copy link
Collaborator

Since this is building off of #193 I'm going to put this on the backburner milestone as well

@CosmicHorrorDev CosmicHorrorDev added this to the v0.8.1 Release milestone Jun 1, 2023
Add config builder, test and support `&str` argument.
@CosmicHorrorDev
Copy link
Collaborator

Alright the release is out, so I'm back to revisiting this. I posted more of my updated thoughts here (#194 (comment)), but the gist is that too many things are currently changing and more stuff is likely to change in the future within sd, so I don't think it's a good idea to split out a separate library crate at the moment

Thanks for your contribution nevertheless, and sorry for changing my mind on this 🙇

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.

None yet

4 participants