Skip to content

Conversation

@JGBSouza
Copy link

This pr add simple comments in the Bookmarked functions improving its documentation.

Copy link
Collaborator

@lorenzoberts lorenzoberts left a comment

Choose a reason for hiding this comment

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

Hey @JGBSouza, thanks for the contribution.

  1. I'm not sure we need to describe implementation details in the docstrings, specially for simple functions, which are fair easy to understand by looking at the implementation. So I would prefer if the function comments were more objective regarding the function purpose.
    Instead of "Add a patchset to the list of bookmarks. The patchset is only added if it is not already on the list.", you could simply say "Add a patchset to the list of bookmarks if it's not bookmarked". The functions are simple, so the docstring should be proportional to that.
  2. Let me know if the "copy" comment makes sense, since it's specific because of Rust details it could be a little counterintuitive.
  3. Regarding the commit message, it's the same thing as the previous PR. We just need 1 prefix, specially for contributions that are relatively simple

Comment on lines 29 to 31
/// Get the patchset that is selected now
///
/// Returns a copy of the selected patchset
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Rust, "copy" probably has a different meaning compared to other languages. I understood what you meant, but it's not super precise in terms of Rust. Since you're clining the Patch, you're not returning a copy of it, but a clone, a.k.a deep copy, which duplicates the Patch in memory

@JGBSouza JGBSouza force-pushed the docs/bookmarked/comments branch 2 times, most recently from 0fc3f30 to 8ef271e Compare November 23, 2025 21:39
This pr add simple comments in the Bookmarked functions improving its documentation.

Signed-off-by: JGBSouza <joaosouzaaa12@usp.br>
@JGBSouza JGBSouza force-pushed the docs/bookmarked/comments branch from 8ef271e to b4cb039 Compare November 23, 2025 21:51
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.

2 participants