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

chore(readme): enhance warning #15076

Closed
wants to merge 5 commits into from
Closed

Conversation

realguse
Copy link

@realguse realguse commented Sep 7, 2024

This PR enhances the README.md by replacing the old warning with a warning that looks nicer.

Before:
image

After:
image

@IQuick143 IQuick143 added C-Docs An addition or correction to our documentation A-Meta About the project itself D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 7, 2024
@janhohenheim
Copy link
Member

Don’t know if this readme is included in any of our docs, but if it is, this will trip up cargo doc, as it will parse it as a hyperlink FYI

@realguse
Copy link
Author

realguse commented Sep 7, 2024

This is just the deafult README.md with the "WARNING" message being replaced.

@janhohenheim
Copy link
Member

I know :) You can do #![doc = include_str!("../README.md")] to include the readme in your rust docs, which lots of crates do. I don't think we do this anywhere in Bevy, but if we did, rust doc would not like the ![WARNING] syntax, as it would resolve to an incomplete link.

@realguse
Copy link
Author

realguse commented Sep 7, 2024

Could you give me the link to the docs where the README is present

@realguse
Copy link
Author

realguse commented Sep 7, 2024

I tested this on other markdown renderers, and there doesn't seem to be any other place than github that renders it.

@janhohenheim
Copy link
Member

Sure! LWIM uses this technique:

Yeah, GitHub is the only place that accepts the ![WARNING] etc. syntax, the other renderers will treat it as a simple (unresolved) link.

@realguse
Copy link
Author

realguse commented Sep 7, 2024

There seems to be some confusion here. This PR is for the README.md in this repo.

@janhohenheim
Copy link
Member

janhohenheim commented Sep 7, 2024

As I said, I don't think Bevy includes the readme anywhere. LWIM is just an example of how it would look like if it did.
All I'm saying is that before merging this, we should make sure that the readme is never included anywhere as documentation.

@realguse
Copy link
Author

realguse commented Sep 7, 2024

Oh, i'm so sorry, ill see if i find anything.

@SludgePhD
Copy link
Contributor

crates.io render the readme too: https://crates.io/crates/bevy

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

I do like using these new annotations. They're prettier and easier on the eyes.

Assuming this README isn't used as documentation on docs.rs or any other renderer, I think we should be fine to use this.

Just a couple typos/suggestions.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@realguse
Copy link
Author

realguse commented Sep 7, 2024

I've gone ahead and fixed the type (>) and added the rust version warning back.

I tried to short it down, but let me know if i should use the original.

@realguse
Copy link
Author

realguse commented Sep 7, 2024

crates.io render the readme too: https://crates.io/crates/bevy

@SludgePhD, do you know if crates.io can render alerts?

README.md Show resolved Hide resolved
@SludgePhD
Copy link
Contributor

@SludgePhD, do you know if crates.io can render alerts?

I don't know, but they are a non-standard feature that isn't supported by most standard markdown engines, so if I had to guess I'd say no.

@realguse
Copy link
Author

realguse commented Sep 7, 2024

@SludgePhD, do you know if crates.io can render alerts?

I don't know, but they are a non-standard feature that isn't supported by most standard markdown engines, so if I had to guess I'd say no.

Is there any way of excluding it from the crates.io readme?

@realguse realguse changed the title chore(readme): replace warning chore(readme): enhance warning Sep 7, 2024
@SludgePhD
Copy link
Contributor

I don't think so. The only thing you can do is to have two readmes, maybe one of them generated from the other to reduce duplication. Too much hassle for this minor change for sure. Or you could remove the crates.io readme entirely.

@janhohenheim
Copy link
Member

janhohenheim commented Sep 7, 2024

Or set up the [WARNING] label to be a valid link, but idk what it would link to. Rick Roll? :p
Overall, I'm against merging this until we can make sure it does not break our crates.io presence.

@realguse
Copy link
Author

realguse commented Sep 8, 2024

The warning could link to the github readme, or (if its possibly) link to the warning alert in the github readme

@realguse
Copy link
Author

realguse commented Sep 8, 2024

the warning could link to a link like this (not rick roll btw)

https://arc.net/l/quote/bvslnemh

as you can see the warning gets highlighted

@realguse realguse marked this pull request as draft September 8, 2024 09:44
@realguse
Copy link
Author

realguse commented Sep 8, 2024

I tried linking to the higlitied text but it turned out like this:

image

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 9, 2024
@realguse
Copy link
Author

Closed because of crates.io.

@realguse realguse closed this Sep 12, 2024
@realguse realguse deleted the patch-1 branch September 12, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Meta About the project itself C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants