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

Use workspace dependencies #2561

Closed
wants to merge 12 commits into from
Closed

Use workspace dependencies #2561

wants to merge 12 commits into from

Conversation

DaviRain-Su
Copy link

No description provided.

@vercel
Copy link

vercel bot commented Jul 9, 2023

@DaviRain-Su is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@Aursen
Copy link
Contributor

Aursen commented Jul 9, 2023

This ensures consistency across crates, but it's not very editor-friendly

@DaviRain-Su
Copy link
Author

DaviRain-Su commented Jul 9, 2023

ye, but I also find some project use this form. https://github.com/Sovereign-Labs/sovereign-sdk

The best thing about it is that it no longer maintains local libraries using the "path" approach. This is very user-friendly.

rust blog: https://blog.rust-lang.org/2022/09/22/Rust-1.64.0.html#cargo-improvements-workspace-inheritance-and-multi-target-builds

@acheroncrypto acheroncrypto added the dependencies Pull requests that update a dependency file label Jul 10, 2023
Copy link
Collaborator

@acheroncrypto acheroncrypto 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, workspace inheritence part of the PR is quite useful but it will be hard to maintain the format with extra spaces because most people will do their thing and won't follow the format.

@DaviRain-Su
Copy link
Author

Please maintain it like this as much as possible. This is the way I find the readability to be the best.

@acheroncrypto
Copy link
Collaborator

Please maintain it like this as much as possible. This is the way I find the readability to be the best.

Is there a script you are running to achieve this? We could only maintain this format if we can automate it otherwise this will cause us to waste a lot of time telling people to follow the format.

I like the format too but time > looks.

@DaviRain-Su
Copy link
Author

I have add dprint.json to format *.toml file. https://dprint.dev/plugins/toml/

@acheroncrypto
Copy link
Collaborator

CI fails because tests are not part of the root workspace(intentional to not use Cargo.lock).

@cereum
Copy link

cereum commented Aug 8, 2023

that's a bummer w/r/t the tests if u want I can drop a code format check action into CI for dprint/rust/ts in either another pr or this one

@acheroncrypto
Copy link
Collaborator

that's a bummer w/r/t the tests if u want I can drop a code format check action into CI for dprint/rust/ts in either another pr or this one

Yeah, the main advantage would be to get rid of the relative paths but it doesn't seem to be possible with the current structure and given most of the dependencies are used only once, I'm not sure if this PR(as it stands) can be merged 😕.

Formatting with dprint could be useful but it would be another tool that people would need to install.

@cereum
Copy link

cereum commented Aug 8, 2023

If the action is only checking for formatting it becomes on the pr author to run 1-2 curl shell-ish scripts to be able to make it go away. But I get it.

@DaviRain-Su DaviRain-Su closed this by deleting the head repository May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants