Skip to content

Add FromRequestParts derive macro#336

Merged
m4tx merged 22 commits into
cot-rs:masterfrom
kumarUjjawal:master
Jun 9, 2025
Merged

Add FromRequestParts derive macro#336
m4tx merged 22 commits into
cot-rs:masterfrom
kumarUjjawal:master

Conversation

@kumarUjjawal

Copy link
Copy Markdown
Contributor

adds the FromRequestParts derive macro implementation.

Closes #316

@github-actions github-actions Bot added the C-macros Crate: cot-macros label May 20, 2025
@codecov

codecov Bot commented May 20, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cot-macros/src/from_request.rs 95.83% 1 Missing ⚠️
cot/src/admin.rs 0.00% 1 Missing ⚠️
Flag Coverage Δ
rust 88.74% <93.33%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cot-macros/src/lib.rs 96.61% <100.00%> (+0.31%) ⬆️
cot/src/request/extractors.rs 92.04% <ø> (ø)
cot-macros/src/from_request.rs 95.83% <95.83%> (ø)
cot/src/admin.rs 73.40% <0.00%> (-0.03%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@seqre seqre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for your contribution! That's a great start, but I think it can be a bit better.

CI is failing due to a removed Clippy lint (match_on_vec_items) in cot/src/utils/graph.rs, which I haven’t modified. Should I remove the #[expect(clippy::match_on_vec_items)] line, or would you prefer to handle it separately?

Regarding your comment from the issue, I've checked the clippy workflow logs and I see no such lint error.

Also, please consider installing pre-commit hook, so your code is automatically formatted!

Comment thread cot-macros/src/lib.rs Outdated

@m4tx m4tx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey, thanks for the contribution! This looks fairly good; please have a look at my comments for some outstanding issues before we'll be ready to merge.

Also, I think it would be useful to use the macro on some existing code, for instance, here:

cot/cot/src/admin.rs

Lines 58 to 71 in f9d7c43

#[derive(Debug)]
struct BaseContext {
urls: Urls,
static_files: StaticFiles,
}
impl FromRequestParts for BaseContext {
async fn from_request_parts(parts: &mut Parts) -> cot::Result<Self> {
let urls = Urls::from_request_parts(parts).await?;
let static_files = StaticFiles::from_request_parts(parts).await?;
Ok(Self { urls, static_files })
}
}
to have an easy way to ensure it works correctly!

Comment thread cot-macros/tests/ui/derive_from_request_parts.rs Outdated
Comment thread cot-macros/src/lib.rs Outdated
Comment thread cot-macros/src/lib.rs
Comment thread cot-macros/src/lib.rs Outdated
Comment thread cot-macros/src/lib.rs Outdated
@kumarUjjawal

kumarUjjawal commented May 27, 2025

Copy link
Copy Markdown
Contributor Author

Hey, thanks for the contribution! This looks fairly good; please have a look at my comments for some outstanding issues before we'll be ready to merge.

Also, I think it would be useful to use the macro on some existing code, for instance, here:

cot/cot/src/admin.rs

Lines 58 to 71 in f9d7c43

#[derive(Debug)]
struct BaseContext {
urls: Urls,
static_files: StaticFiles,
}
impl FromRequestParts for BaseContext {
async fn from_request_parts(parts: &mut Parts) -> cot::Result<Self> {
let urls = Urls::from_request_parts(parts).await?;
let static_files = StaticFiles::from_request_parts(parts).await?;
Ok(Self { urls, static_files })
}
}

to have an easy way to ensure it works correctly!

Hi, thank you for taking the time to review and provide valuable feedbacks. I will work on the issues as per your guidelines. This is my first time contributing on a project so please mind my messiness.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx Hi, I am having some issue with module exports, can I push the code so you can take a look.

@m4tx

m4tx commented May 28, 2025

Copy link
Copy Markdown
Member

@m4tx Hi, I am having some issue with module exports, can I push the code so you can take a look.

Sure!

@github-actions github-actions Bot added the C-lib Crate: cot (main library crate) label May 28, 2025
@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx Hi, did you get the time to look at the code for what I am doing wrong?

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx i made some changes and ran tests, all passed.

Comment thread cot-macros/src/from_request.rs
Comment thread cot/src/lib.rs Outdated
Comment thread cot/src/admin.rs Outdated
Comment thread cot-macros/tests/compile_tests.rs
@m4tx

m4tx commented May 31, 2025

Copy link
Copy Markdown
Member

@m4tx i made some changes and ran tests, all passed.

Please have a look at the pipelines, the tests are failing because you're trying to use axum via cot::axum, but we don't reexport it (for good reasons).

I'm not sure how you are running the tests, but running just tests (assuming you have the just runner installed) should run all the tests that do not require database or other external dependencies, and you should see the errors locally.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx should pass now
cot

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx i made some changes and ran tests, all passed.

Please have a look at the pipelines, the tests are failing because you're trying to use axum via cot::axum, but we don't reexport it (for good reasons).

I'm not sure how you are running the tests, but running just tests (assuming you have the just runner installed) should run all the tests that do not require database or other external dependencies, and you should see the errors locally.

I don't know why it was not picking up the errors then I ran cargo +nightly nextest run -p cot_macros --all-features -- derive_from_struct and was able to find the issues.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx added more tests, should cover required patch coverage.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx how can I test the coverage locally so I only push the changes when I am certain it will be above the required.

@seqre

seqre commented Jun 3, 2025

Copy link
Copy Markdown
Member

@m4tx how can I test the coverage locally so I only push the changes when I am certain it will be above the required.

@kumarUjjawal, first, generate the coverage data:

cargo llvm-cov --all-features --workspace --branch --doctests --codecov --output-path codecov.json -- --include-ignored

Then, you can use llvm-cov show or any other tool you like to view code coverage.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@seqre thank you!

@m4tx

m4tx commented Jun 4, 2025

Copy link
Copy Markdown
Member

@seqre @kumarUjjawal FYI there's a just command defined just (pun intended) to do that:

cot/justfile

Lines 41 to 44 in c7e295f

coverage:
# generate coverage report as HTML
# requires cargo-llvm-cov installed and nightly toolchain
cargo llvm-cov --all-features --workspace --branch --doctests --html --open

It also opens the coverage viewer in a web browser automatically.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx All checks are passing now, let me know if there’s anything else you'd like me to do.

@seqre

seqre commented Jun 7, 2025

Copy link
Copy Markdown
Member

@kumarUjjawal, I think there are still some leftover review comments. Please review all and either resolve them if done or apply the suggested solutions.

@m4tx m4tx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 on what @seqre says. This looks very good; please make sure all review comments are addressed and this will be ready to be merged.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

+1 on what @seqre says. This looks very good; please make sure all review comments are addressed and this will be ready to be merged.

Thank you! I've just one review changes left to test. I'm working on that.

@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx @seqre I have made changes according to the review, please check!

@m4tx m4tx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks very good, thanks a lot for the contribution and your patience in waiting for our reviews!

@m4tx m4tx merged commit c0bb892 into cot-rs:master Jun 9, 2025
22 checks passed
@cotbot cotbot Bot mentioned this pull request Jun 5, 2025
@kumarUjjawal

Copy link
Copy Markdown
Contributor Author

@m4tx thank you for your help. I hope to contribute more to this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-lib Crate: cot (main library crate) C-macros Crate: cot-macros

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add FromRequestParts derive macro

3 participants