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

Feat/inplace: add new parameter --inplace (-i) for in-place formatting #377

Merged
merged 10 commits into from
Apr 18, 2024

Conversation

bioinformatist
Copy link
Contributor

@bioinformatist bioinformatist commented Apr 18, 2024

Some clippy lints also have been fixed separately 😺

@kivikakk
Copy link
Owner

Thank you, this looks excellent.

I really appreciate the extra clippy fixes! I know in the past that some of these changes caused issues for people because it caused an increase in the MSRV (see #290), but I think it's been long enough that I'm happy to move on. Still, before I merge, I'm going to compare the lib and bin MSRVs before and after this change, just for my own reference, and make an appropriate declaration in Cargo.toml.

Thank you again! 🤍

@kivikakk
Copy link
Owner

OK! in-place itself specifies an MSRV of 1.70 (or 1.65 for in-place 0.1.0), but I don't really care about the binary's MSRV. I've made in-place an optional dependency and added it to the cli feature.

Now we get an MSRV of 1.62.1, compared with 1.60.0 on main. This seems completely fine to me — thanks again!

@kivikakk kivikakk enabled auto-merge April 18, 2024 12:27
@kivikakk kivikakk merged commit 15609f7 into kivikakk:main Apr 18, 2024
12 checks passed
@digitalmoksha
Copy link
Collaborator

@bioinformatist just curious, is there any difference between

cargo run -- --inplace test.md

and

cargo run -- --to commonmark --output test.md test.md

It seems to work the same for me 🤔

@kivikakk
Copy link
Owner

It is the same, and it should be! And it's fine for it to be 👍 We could expand -i to allow specifying multiple files in future, or directories.

@digitalmoksha
Copy link
Collaborator

Sounds good to me. One thought I had was if the option was set we might be able to just set the proper to and output options internally. But then we wouldn't be able to easily expand into multiple files, which would be desirable.

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.

3 participants