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

fix docs format #2497

Merged
merged 8 commits into from
Aug 11, 2024
Merged

fix docs format #2497

merged 8 commits into from
Aug 11, 2024

Conversation

yihau
Copy link
Member

@yihau yihau commented Aug 8, 2024

(part of #2487)

Problem

Screenshot 2024-08-06 at 23 56 02

docs take a stricter approach to indentation rules…

Summary of Changes

fix it

@yihau yihau requested a review from steviez August 8, 2024 16:36
@yihau yihau marked this pull request as ready for review August 8, 2024 16:36
steviez
steviez previously approved these changes Aug 8, 2024
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Two minor nitpicks, but on second thought, maybe we shouldn't be making those changes in this PR. So, I'd say go ahead and ship it

//! The ciphertext can be generalized to hold not a single decryption handle, but multiple handles
//! pertaining to multiple ElGamal public keys. These ciphertexts are referred to as a "grouped"
//! ElGamal ciphertext.
//! The ciphertext can be generalized to hold not a single decryption handle, but multiple handles
Copy link

Choose a reason for hiding this comment

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

Nit, the first line above the diff should probably have a period at the end
to a specific public key TO to a specific public key.

//! In contrast to the traditional ElGamal encryption scheme, the twisted ElGamal encodes messages
//! directly as a Pedersen commitment. Therefore, proof systems that are designed specifically for
//! Pedersen commitments can be used on the twisted ElGamal ciphertexts.
//! In contrast to the traditional ElGamal encryption scheme, the twisted ElGamal encodes messages
Copy link

Choose a reason for hiding this comment

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

Nit, the first line above the diff should probably have a period at the end
to a specific public key TO to a specific public key.

Comment on lines 216 to 231
/// from the output row. An example of a true filter is the `value_regex_filter`,
/// which excludes cells whose values don't match the specified pattern. All
/// regex true filters use RE2 syntax (<https://github.com/google/re2/wiki/Syntax>)
/// in raw byte mode (RE2::Latin1), and are evaluated as full matches. An
/// important point to keep in mind is that `RE2(.)` is equivalent by default to
/// `RE2(\[^\n\])`, meaning that it does not match newlines. When attempting to
/// match an arbitrary byte, you should therefore use the escape sequence `\C`,
/// which may need to be further escaped as `\\C` in your client language.
///
/// * Transformers alter the input row by changing the values of some of its
/// cells in the output, without excluding them completely. Currently, the only
/// supported transformer is the `strip_value_transformer`, which replaces every
/// cell's value with the empty string.
/// cells in the output, without excluding them completely. Currently, the only
/// supported transformer is the `strip_value_transformer`, which replaces every
/// cell's value with the empty string.
///
/// * Chains and interleaves are described in more detail in the
/// RowFilter.Chain and RowFilter.Interleave documentation.
/// RowFilter.Chain and RowFilter.Interleave documentation.

Choose a reason for hiding this comment

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

This is an auto-generated file, so the next time we have to update it, there's a decent chance we hit this lint again 😕 Can we ignore the lint for this module?

Copy link

Choose a reason for hiding this comment

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

Good callout. Hypothetically, we could wait for that crate to update (assuming they pay attention to lints) or PR the crate ourselves

Copy link

@steviez steviez Aug 8, 2024

Choose a reason for hiding this comment

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

Or maybe we can ignore the lint for just this crate (like Tyera suggested above) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry if I asked this before, but could you tell me how we generate those files? like what are the commands or libs we use? I guess it will be better if we can have a patch for the upstream 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

btw, I removed this docs change from this PR: e01da6e looks like it's worth to have another PR for discussing this one haha. will ping you guys there!

Copy link

Choose a reason for hiding this comment

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

This is an auto-generated file, so the next time we have to update it, there's a decent chance we hit this lint again

It looks like this might be the case for the changes made in: #1516

I removed this docs change from this PR

This seems like a good compromise for this PR.

sorry if I asked this before, but could you tell me how we generate those files? like what are the commands or libs we use?

I believe this script will do it:
https://github.com/anza-xyz/agave/blob/master/storage-bigtable/build-proto/build.sh

Running that script + doing git diff, I see some items that were removed in 1516 are added again. Seeing this, I'm now more in favor of Tyera's suggestion for ignoring more lints for these couple files

Choose a reason for hiding this comment

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

Ack, I forgot to follow up on 1516 and re-add that stuff.
Yes, the link Steve posted is the correct build script. We are using tonic_build to generate the files from the googleapi protobuf files, so we commit new files when that crate updates. There are a couple doc blocks that we have been manually adding ignore to, although we could potentially make the docs CI steps ignore these files, or pick some other approach instead.

Anyway, thanks for removing the google file from this PR.

@yihau
Copy link
Member Author

yihau commented Aug 9, 2024

sorry, I think I committed some wrong indent and broke the original meaning. just read the docs again and pushed some updates. 🫠

Copy link

mergify bot commented Aug 9, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

steviez
steviez previously approved these changes Aug 9, 2024
@yihau
Copy link
Member Author

yihau commented Aug 9, 2024

force-pushed for fixing the conflict 🫠

@yihau yihau merged commit 3838ee0 into anza-xyz:master Aug 11, 2024
51 checks passed
@yihau yihau deleted the fix-docs-2 branch August 11, 2024 09:14
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