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: Add support section #133

Closed
wants to merge 6 commits into from

Conversation

tisonkun
Copy link
Contributor

This refers to #54

@tisonkun
Copy link
Contributor Author

cc @epage

@tisonkun tisonkun mentioned this pull request Apr 16, 2024
@tisonkun tisonkun force-pushed the add-supports-links branch 3 times, most recently from 2ec81f6 to f773eb4 Compare April 16, 2024 11:09
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tisonkun tisonkun force-pushed the add-supports-links branch from f773eb4 to 624b886 Compare April 16, 2024 11:18
@tisonkun tisonkun force-pushed the add-supports-links branch from 624b886 to e487206 Compare April 16, 2024 11:19
@tisonkun tisonkun changed the title Add support links feat: Add support links Apr 16, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
rust-toolchain.toml Outdated Show resolved Hide resolved
@epage epage changed the title feat: Add support links feat: Add support section Apr 16, 2024
src/lib.rs Outdated
@@ -64,6 +64,8 @@ pub struct Metadata {
pub authors: Cow<'static, str>,
/// The URL of the crate's website
pub homepage: Cow<'static, str>,
/// The support information
pub supports: Cow<'static, str>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this should be support rather than supports, both in code and in the UI

src/lib.rs Outdated
@@ -64,6 +64,8 @@ pub struct Metadata {
pub authors: Cow<'static, str>,
/// The URL of the crate's website
pub homepage: Cow<'static, str>,
/// The support information
pub supports: Cow<'static, str>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably be using Option<_> rather than using an empty string as a sentinel

Copy link
Collaborator

Choose a reason for hiding this comment

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

(yes, we check for empty strings in other cases but those are meant to be populated by metadata! and likely we shouldn't even be doing that...)

src/lib.rs Outdated Show resolved Hide resolved
@epage
Copy link
Collaborator

epage commented Apr 16, 2024

Thanks for moving this forward!

@tisonkun tisonkun force-pushed the add-supports-links branch from 7f89425 to f28307b Compare April 16, 2024 21:34
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@tisonkun
Copy link
Contributor Author

Anyway push a solution. Please feel free to drop more comments.

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@epage
Copy link
Collaborator

epage commented Apr 17, 2024

I've just merged a PR for updating the CI and another PR for making the API more resilient to change.

@tisonkun
Copy link
Contributor Author

@epage Then I suppose you can pick up this PR onto your edits?

Close as conflict now.

@tisonkun tisonkun closed this Apr 17, 2024
@tisonkun tisonkun deleted the add-supports-links branch April 17, 2024 02:46
@tisonkun
Copy link
Contributor Author

I can redo it, but you go further on this way already now. I don't care the credit but get the changes done. I suppose you can go faster than me.

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