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.

Sign up for GitHub

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

Jump to bottom

Mz/enable all doctest warnings #1418

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Mz/enable all doctest warnings #1418

merged 3 commits into from
Sep 12, 2024

Conversation

mayeul-zama
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 25, 2024
@mayeul-zama mayeul-zama force-pushed the mz/enable_all_doctest_warnings branch 2 times, most recently from dcc974b to 2c893ff Compare July 25, 2024 12:16
IceTDrinker
IceTDrinker reviewed Jul 25, 2024
View reviewed changes
tfhe/docs/fundamentals/encrypt-data.md Outdated
Comment on lines 16 to 22
let (client_key, server_key) = generate_keys(config);
let (client_key, _server_key) = generate_keys(config);

let clear_a = 27u8;
let clear_b = 128u8;

let a = FheUint8::encrypt(clear_a, &client_key);
let b = FheUint8::encrypt(clear_b, &client_key);
let _a = FheUint8::encrypt(clear_a, &client_key);
let _b = FheUint8::encrypt(clear_b, &client_key);
Copy link
Member

Choose a reason for hiding this comment

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

let's keep the allow dead code in the tfhe/src/test_user_docs.rs otherwise the outside facing documentation will look very weird

IceTDrinker
IceTDrinker reviewed Jul 25, 2024
View reviewed changes
tfhe/docs/getting_started/quick_start.md Outdated
@@ -59,5 +59,6 @@ Refer to the [installation documentation](installation.md) for configuration opt
The `prelude` pattern provides a convenient way to globally import all important **TFHE-rs** traits at once. This approach saves time and avoids confusion.

```rust
#[allow(unused_imports)]
Copy link
Member

Choose a reason for hiding this comment

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

allow that too for the .md docs

IceTDrinker
IceTDrinker reviewed Jul 25, 2024
View reviewed changes
tfhe/src/boolean/client_key/mod.rs Outdated
Comment on lines 144 to 143
/// let cks = ClientKey::new(&PARAMETERS_ERROR_PROB_2_POW_MINUS_165);
/// let _cks = ClientKey::new(&PARAMETERS_ERROR_PROB_2_POW_MINUS_165);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this makes sense for doctests

IceTDrinker
IceTDrinker reviewed Jul 25, 2024
View reviewed changes
tfhe/src/lib.rs
Comment on lines +5 to +7
// Enable all warnings in doctests
// https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#showing-warnings-in-doctests
#![doc(test(attr(warn(unused))))]
Copy link
Member

Choose a reason for hiding this comment

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

so as I said somewhere else, let's not have that in the user facing docs, otherwise it just looks ridiculous, I'm not convinced for doctests in general either, it looks very bad to have to essentially to prefix everything with _, but I'm guessing we are missing the unused imports if we allow that in that case ?

@mayeul-zama mayeul-zama force-pushed the mz/enable_all_doctest_warnings branch 3 times, most recently from dab66f2 to 2eeb8dc Compare July 26, 2024 10:09
IceTDrinker
IceTDrinker requested changes Jul 26, 2024
View reviewed changes
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Keep the changes that are relevant but we don't want to have to rename our variables for doc tests and our md docs

@mayeul-zama mayeul-zama force-pushed the mz/enable_all_doctest_warnings branch 2 times, most recently from 85389e1 to 5098e47 Compare September 11, 2024 11:58
IceTDrinker
IceTDrinker reviewed Sep 11, 2024
View reviewed changes
tfhe/docs/getting_started/quick_start.md Outdated
@@ -59,5 +59,6 @@ Refer to the [installation documentation](installation.md) for configuration opt
The `prelude` pattern provides a convenient way to globally import all important **TFHE-rs** traits at once. This approach saves time and avoids confusion.

```rust
# #[allow(unused_imports)]
Copy link
Member

Choose a reason for hiding this comment

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

does this show up in the gitbook doc ?

unfortunately in the .md files we should not have allow attributes otherwise people reading the doc will see those appear in the HTML most likely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this show up in the gitbook doc ?

Indeed

unfortunately in the .md files we should not have allow attributes otherwise people reading the doc will see those appear in the HTML most likely

It seems like it's not possible to ignore doctest warnings by module

Copy link
Member

Choose a reason for hiding this comment

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

the global lib warn cannot be overriden later ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently not
I tried a few different ways but it didn't work

Copy link
Member

Choose a reason for hiding this comment

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

can it be added in modules except docs ? like in high_level, integer, shortint, core ? or is it a always a lib attribute ?

Copy link
Member

Choose a reason for hiding this comment

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

it's an attribute so unless we do it on a per module basis I don't think we can undo it, what a lame way to enable stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/nightly/rustdoc/write-documentation/the-doc-attribute.html#testattr
It's in the section "At the crate level"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error says "error: this attribute can only be applied at the crate level"

Copy link
Member

Choose a reason for hiding this comment

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

🙄 what a weird design

IceTDrinker
IceTDrinker approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

Thanks a lot !

Please don't forget to create a follow up issue :)

@zama-bot zama-bot removed the approved label Sep 11, 2024
@IceTDrinker IceTDrinker self-requested a review September 12, 2024 08:30
@mayeul-zama mayeul-zama merged commit bd21971 into main Sep 12, 2024
86 of 88 checks passed
@mayeul-zama mayeul-zama deleted the mz/enable_all_doctest_warnings branch September 12, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IceTDrinker IceTDrinker IceTDrinker approved these changes

Assignees
No one assigned
Labels
approved cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mayeul-zama @IceTDrinker @zama-bot