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(docs): Rust guide #107

Merged
merged 16 commits into from
Nov 28, 2023
Merged

feat(docs): Rust guide #107

merged 16 commits into from
Nov 28, 2023

Conversation

Mr-Leshiy
Copy link
Contributor

@Mr-Leshiy Mr-Leshiy commented Nov 24, 2023

Description

  • Added Rust project building guide.
  • Moved example Rust project to the examples/rust dir.

Related Issue(s)

Closes #78

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@Mr-Leshiy
Copy link
Contributor Author

Mr-Leshiy commented Nov 24, 2023

Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

Sorry, you have made a ton of unrelated changes to the code outside of the scope for the ticket.
The ticket did not ask you to rename +BUILDER to +SETUP that is disturbing to anyone else working on the code base, and also breaks cat-voices.
Even if that's a change we want to make and I don't know why we would, that needs to be a ticket all its own, and each target should be addressed separately.
Please revert all changes that have nothing to do with Rust documentation.

The task also was not to refactor or re-write the rust builder. So again, revert all that.
If you think the rust builder needs correction it needs to be raised in a ticket and properly discussed.

I have things in the builder for reasons. Especially to help us debug if things go wrong.
It doesn't;t cost us anything to print out the build platform data at the start of setting up the container.

Your setup assumes that rust-toolchain is with the Earthfile, it may not be.
There are so many changes to the rust builder that I can;t reason through it.
Your task was not to re-write it.
It was to document it ONLY.

I will not approve this PR unless it has only the following:

  1. Rust documentation.
  2. Fixes to demonstratable issues discovered while documenting it.
  3. You CAN move the rust example to the examples folder, but it should just be a move, and the changes in it should be limited to just that move (paths changing for example).

Given the work entailed in reverting, it is probably a lot faster to just re-branch the code from master, add the docs edited to suit the actual rust builder, and move the example. I strongly recommend this is what you do.

@Mr-Leshiy Mr-Leshiy requested a review from stevenj November 28, 2023 09:07
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

See comments for details.

earthly/docs/Readme.md Outdated Show resolved Hide resolved
earthly/python/Readme.md Outdated Show resolved Hide resolved
earthly/rust/Earthfile Show resolved Hide resolved
earthly/rust/Earthfile Show resolved Hide resolved
docs/src/guides/languages/rust.md Outdated Show resolved Hide resolved
earthly/rust/Earthfile Show resolved Hide resolved
examples/rust/Earthfile Outdated Show resolved Hide resolved
@Mr-Leshiy Mr-Leshiy enabled auto-merge (squash) November 28, 2023 11:20
@Mr-Leshiy Mr-Leshiy requested a review from stevenj November 28, 2023 12:05
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

OK

@Mr-Leshiy Mr-Leshiy merged commit b9f4695 into master Nov 28, 2023
45 checks passed
@Mr-Leshiy Mr-Leshiy deleted the feat/rust-guide branch November 28, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add Guide for Rust
2 participants