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: add optional support for serde, dockerfile and HTTP API #56

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

Conversation

quesurifn
Copy link
Owner

No description provided.

@quesurifn
Copy link
Owner Author

Maybe I was a bit too cavalier blindly accepting my IDE's suggestions so there are some unintended consequences. Please let me know if they make a material difference; if they do I'll change them.

@quesurifn
Copy link
Owner Author

Closes #41 and #32

@quesurifn quesurifn changed the title feat: add optional support for serde feat: add optional support for serde, dockerfile and HTTP API Mar 16, 2025
COPY ./Cargo.toml ./Cargo.toml
COPY ./Cargo.lock ./Cargo.lock

# Create empty source files to trick cargo into building our dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? I don't get this. You are actually copying these things further down?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The copy statements below and here are not the same? Do you mind elaborating?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why first "create empty source files" and then copy the real source files? Why not just copy the source files? It says "trick cargo", but I don't get why or what that means.

COPY ./yake_rust/benches ./yake_rust/benches
COPY ./server/src ./server/src
COPY ./python/Cargo.toml ./python/
COPY ./python/src ./python/src
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we are not installing some binary, but rather building everything from scratch in the Dockerfile, so could we get away with not having the python bindings in the Dockerfile? I mean, they are presumably not used and never intended to be used?

Or would we maybe need to make python bindings a "feature" for that?

Copy link
Owner Author

@quesurifn quesurifn Mar 18, 2025

Choose a reason for hiding this comment

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

There was some sort of issue where it seemed the tests in core Yake were being run by python. I'll look into it more and ping you once I have.

@@ -14,6 +14,7 @@ repository = "https://github.com/quesurifn/yake-rust"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[features]
serde = ["dep:serde"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can omit it. serde = { optional = true } automatically allocates a feature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file probably belongs to the /server folder

@xamgore
Copy link
Collaborator

xamgore commented Mar 18, 2025

I am a bit curious whether LLM was used while working on the issue? :)

@quesurifn
Copy link
Owner Author

quesurifn commented Mar 18, 2025

@xamgore For docker, READMEs and cargo.toml yes. Remember the last thing I wrote in Rust was this. I don't really know the language / tool chain yet which includes compiler optimizations and such.

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.

3 participants