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

Support cargo workspace inheritance #98

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

Conversation

yassun7010
Copy link

@yassun7010 yassun7010 commented Jan 8, 2024

Related #81.

I rewrite using cargo_toml crate for workspace inheritance.

pub fn github(badge: cargo_toml::Badge) -> String {
let repo = badge.repository;
// TODO: support workflow option.
let workflow = "main"; // BADGE_WORKFLOW_DEFAULT
Copy link
Author

Choose a reason for hiding this comment

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

That's a problem. cargo_toml does not support [badge.github] field.

But badge section is deprecated, so I am hesitant to make a PR to cargo_toml.

Copy link

@Freyskeyd Freyskeyd Mar 13, 2024

Choose a reason for hiding this comment

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

The badge should be removed from cargo-readme following cargo_toml recommandation.
The badges, if the maintainer want them, should be placed into a template (README.tpl) like that:

# {{crate}}

![Test workflow](https://github.com/topos-protocol/topos/actions/workflows/test.yml/badge.svg)
![Quality workflow](https://github.com/topos-protocol/topos/actions/workflows/quality.yml/badge.svg)
![MSRV](https://img.shields.io/badge/MSRV-1.71.1-blue?labelColor=1C2C2E&logo=Rust)

{{readme}}

@webern what do you think of this solution? I think it should considered as breaking change tho

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good to me. Fewer things for this code to worry about.

Copy link
Author

Choose a reason for hiding this comment

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

@webern

Are there any modifications I can make to this PR?

Choose a reason for hiding this comment

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

I don't think there is anything you can do without breaking

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

What can I do to merge this PR?

Choose a reason for hiding this comment

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

Hello,

For this PR I would completely remove the badges from the generation and update the doc to explain how to add them in the README.tpl, it is a breaking but I don't see another alternative. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

It would be good.

The Cargo.toml spec is changing and badges are already deprecated.

@yassun7010 yassun7010 changed the title Support cargo workspace Support cargo workspace inheritance Jan 8, 2024
@yassun7010 yassun7010 requested a review from webern April 3, 2024 03:49
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