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

Refactor fn_macros to rune-macros #44

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

Qkessler
Copy link
Contributor

@Qkessler Qkessler commented Dec 7, 2023

Change Summary

The reasoning behind this is that this is the start of the modularization of the general crate, where I'm following the examples structure of some famous crates: axum (library) and ripgrep (binary). Axum has an axum preffix behind all the crates its main packages uses, so thought it would be beneficial to do the same.

Also, moved rune-macros (renamed) inside crates, as it's a crate we depend on, and we already had one there. It's a good practice to list the members of the workspace directly, instead of using the wildcard, to avoid accidential build inclusion.

Lastly, added documentation to the rune-macros crate, to make sure that other contributors can check the crate and understand its usage from the documentation. Haven't documented Trace since I'm not totally sure of what we want to include there or the usage of Trace. Please, document it in another commit.

Risks associated with this change

None that I think of.

Testing

cargo doc has the right output, links are correct. For the future, REVIEW: we can add CI for cargo doc running on the crates we plan to refactor: rune-core, rune-macros and others. We'll see CI as part of this PR.

The reasoning behind this is that this is the start of the
modularization of the general crate, where I'm following the examples
structure of some famous crates: axum (library) and
ripgrep (binary). Axum has an `axum` preffix behind all the crates its
main packages uses, so thought it would be beneficial to do the same.

Also, moved rune-macros (renamed) inside `crates`, as it's a crate we
depend on, and we already had one there. It's a good practice to list
the members of the workspace directly, instead of using the wildcard, to
avoid accidential build inclusion.
@Qkessler
Copy link
Contributor Author

Qkessler commented Dec 7, 2023

For the CI, I don't quite understand why test on ubuntu and mac pass, but not on windows.

@CeleritasCelery
Copy link
Owner

it broke because we moved the new crate after the [target.'cfg(not(target_env = "msvc"))'.dependencies] in Cargo.toml. That meant that is was not included on windows.

@Qkessler
Copy link
Contributor Author

Qkessler commented Dec 7, 2023

Damn, that was a surprise. Thanks!

@CeleritasCelery CeleritasCelery merged commit 46fd144 into CeleritasCelery:master Dec 7, 2023
13 checks passed
@CeleritasCelery
Copy link
Owner

Thank you! This looks great.

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