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

Update hello world example to use repo on crates.io #51

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

Conversation

kyle-watson
Copy link

This is my first ever pull request, so please let me know if I messed anything up or if this isn't even a desired change. Also I am not positive if the Linux log is accurate as I compiled on windows and I replaced the parts of the log that looked similar.

@Bromeon Bromeon added the outdated-api Information that is no longer accurate label Jul 8, 2024
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thank you! It's a good change, we should probably use the opportunity to clean this up a bit, see comments.

Comment on lines 92 to 98
```log
$ cargo build
Compiling godot4-prebuilt v0.0.0
(https://github.com/godot-rust/godot4-prebuilt?branch=4.1.1#fca6897d)
Compiling proc-macro2 v1.0.69
[...]
Compiling godot v0.1.0 (https://github.com/godot-rust/gdext?branch=master#66df8f47)
Compiling godot-cell v0.1.1
Compiling glam v0.27.0
Compiling godot-core v0.1.1
Compiling godot-ffi v0.1.1
Compiling godot v0.1.1
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good example why we shouldn't include the log. It will already be outdated on next crates.io release again, and anytime we change dependencies, it becomes inaccurate. Furthermore, the output differs depending on the number of crates that have changed (it's not a full rebuild).

Could you remove the whole cargo build log?

The part about ls -l is also a bit strange, because ls doesn't typically show nested directories, nor does it put them in a frame. Maybe we should use a more conventional output here, following a ls -l target/debug command. But we can also just keep it OS-independent with a paragraph like

After building, you should have the following files in the target/debug directory. Note that this applies to Linux, on Windows and macOS you will have different dynamic library names and file extensions.

Comment on lines 75 to 77
[dependencies]
godot = { git = "https://github.com/godot-rust/gdext", branch = "master" }
godot = "0.1"
```
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep both versions, for users who want a crates release and users who want to follow bleeding-edge development. Maybe mention the crates.io one first, and then introduce the GitHub one.

@Bromeon
Copy link
Member

Bromeon commented Jul 18, 2024

@kyle-watson Any update? 🙂

@Bromeon
Copy link
Member

Bromeon commented Jul 18, 2024

Can we maybe recommend using cargo add godot instead of specifying the dependeny manually in Cargo.toml? Not only is that good practice, but it also prevents us from manually updating this page on every minor release...

@@ -91,7 +93,7 @@ This should output to `{YourCrate}/target/debug/` at least one variation of a co

```admonish tip
For users who want to follow bleeding-edge development, you can directly link the github repo in your `Cargo.toml` dependencies section by replacing:
`godot = "0.1"`
`godot = "X.X.X"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`godot = "X.X.X"`
`godot = "0.x.y"`

So it's clear that the parts are different.

I think it's fair to update this if we get to version 1 😉

@Bromeon
Copy link
Member

Bromeon commented Aug 3, 2024

@kyle-watson Do you have an update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
outdated-api Information that is no longer accurate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants