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

Add Rust Docs CI #1271

Closed
wants to merge 2 commits into from
Closed

Add Rust Docs CI #1271

wants to merge 2 commits into from

Conversation

plebhash
Copy link
Collaborator

@plebhash plebhash commented Dec 5, 2024

close #1270

this PR does two things:

  • enforce #![deny(missing_docs)] on every *.rs file that still doesn't have it
  • enforce CI does cargo doc --all-features on every crate of the repo (via shell recursion)

we should aim to merge this PR by the end of Milestone 1.2.0

this will be the final toothcomb that will give us confidence to move forward from #845 knowing that we didn't let anything go without notice

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.27%. Comparing base (67a3f00) to head (ec9eb50).
Report is 9 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (67a3f00) and HEAD (ec9eb50). Click for more details.

HEAD has 21 uploads less than BASE
Flag BASE (67a3f00) HEAD (ec9eb50)
bip32_derivation-coverage 1 0
key-utils-coverage 1 0
binary_sv2-coverage 1 0
binary_serde_sv2-coverage 1 0
codec_sv2-coverage 1 0
common_messages_sv2-coverage 1 0
framing_sv2-coverage 1 0
job_declaration_sv2-coverage 1 0
noise_sv2-coverage 1 0
roles_logic_sv2-coverage 1 0
v1-coverage 1 0
sv2_ffi-coverage 1 0
template_distribution_sv2-coverage 1 0
mining-coverage 1 0
roles 1 0
jd_client-coverage 1 0
jd_server-coverage 1 0
mining_device-coverage 1 0
mining_proxy_sv2-coverage 1 0
pool_sv2-coverage 1 0
translator_sv2-coverage 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #1271       +/-   ##
==========================================
- Coverage   19.30%   4.27%   -15.03%     
==========================================
  Files         164      94       -70     
  Lines       10849    6079     -4770     
==========================================
- Hits         2094     260     -1834     
+ Misses       8755    5819     -2936     
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage ?
binary_sv2-coverage ?
bip32_derivation-coverage ?
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage ?
common_messages_sv2-coverage ?
const_sv2-coverage 0.00% <ø> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage ?
jd_client-coverage ?
jd_server-coverage ?
job_declaration_sv2-coverage ?
key-utils-coverage ?
mining-coverage ?
mining_device-coverage ?
mining_proxy_sv2-coverage ?
noise_sv2-coverage ?
pool_sv2-coverage ?
protocols 0.00% <ø> (-24.73%) ⬇️
roles ?
roles_logic_sv2-coverage ?
sv2_ffi-coverage ?
template_distribution_sv2-coverage ?
translator_sv2-coverage ?
utils 25.02% <ø> (-0.11%) ⬇️
v1-coverage ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

A couple of general notes:

  1. I think at this point we should just have a single rust.yml file in the CI and do all the rust-related stuff, like building/testing/docs/msrv.. It is getting crowded here because we add a new file for each action
  2. I think it should be enough to add the deny macro to main/lib, it should be reflected across the crate?

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 7, 2024

A couple of general notes:

  1. I think at this point we should just have a single rust.yml file in the CI and do all the rust-related stuff, like building/testing/docs/msrv.. It is getting crowded here because we add a new file for each action
  2. I think it should be enough to add the deny macro to main/lib, it should be reflected across the crate?

1 would be good on a separate PR

2 is interesting feedback, but the engineering effort is higher, because we would need human eyes going with a fine comb over each module

do you feel strongly about this?

the current approach is naive and not elegant, but it gets the job done without significant negative consequences

@jbesraa
Copy link
Contributor

jbesraa commented Dec 7, 2024

for (1) #1272

with (2), what is the difference when you include the flag in every files?

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 9, 2024

closing this

after trying to implement all the changes on roles_logic_sv2 I quickly realized that the engineering effort here is not worth it

introducing the deny(missing_docs) macro, A LOT of new errors come up

to make the CI happy, we would need to spend too much human hours adding redundant docs


I'm not against having this CI... if someone is opinionated and wants to spend time on making this PR ready, feel free to spend time on it (e.g.: @Shourya742 )

I just don't want to spend my time here, I feel there's higher prio things to be done right now

@plebhash plebhash closed this Dec 9, 2024
@Fi3
Copy link
Collaborator

Fi3 commented Dec 9, 2024 via email

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.

Rust Docs CI
3 participants