-
Notifications
You must be signed in to change notification settings - Fork 17
updated docstrings #170
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
updated docstrings #170
Conversation
basfroman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you've already started correcting your docstrings, let's make it perfect.
- Use the same name for arguments (Args or Arguments - presumably the latter).
- If a function has an parameter, add its description in the variable docstring as well.
- Use consistent indentation throughout the all updated docstring description.
| /// Checks for existing coldkeypub and hotkeys, and creates them if non-existent. | ||
| /// Arguments: | ||
| /// coldkey_use_password (bool): Whether to use a password for coldkey. Defaults to ``True``. | ||
| /// hotkey_use_password (bool): Whether to use a password for hotkey. Defaults to ``False``. | ||
| /// save_coldkey_to_env (bool): Whether to save a coldkey password to local env. Defaults to ``False``. | ||
| /// save_hotkey_to_env (bool): Whether to save a hotkey password to local env. Defaults to ``False``. | ||
| /// coldkey_password (Optional[str]): Coldkey password for encryption. Defaults to ``None``. If `coldkey_password` is passed, then `coldkey_use_password` is automatically ``True``. | ||
| /// hotkey_password (Optional[str]): Hotkey password for encryption. Defaults to ``None``. If `hotkey_password` is passed, then `hotkey_use_password` is automatically ``True``. | ||
| /// overwrite (bool): Whether to overwrite an existing keys. Defaults to ``False``. | ||
| /// suppress (bool): If ``True``, suppresses the display of the keys mnemonic message. Defaults to ``False``. | ||
| /// | ||
| /// Returns: | ||
| /// Wallet instance with created keys. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind to add additional docstrings, but I don't want to remove existing ones.
Actually, I care about the code quality first then about the documentation.
We need to find the way, when both goals achieved, but without code|docstrings editing|removing bc some doc generator doesn't work properly.
Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know what? let me try putting that bit of docstrings back in the python_bindings.rs file but with the updated formatting i adopted for the docstrings in the create function in the wallet.rs file.
i'll let you know how that turns out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works fine with the new format. i've made pushed the latest changes
| /// Checks if the given public_key is a valid ed25519 key. | ||
| /// | ||
| /// Arguments: | ||
| /// public_key (bytes): The public_key to check as bytes. | ||
| /// Returns: | ||
| /// valid (bool): ``True`` if the public_key is a valid ed25519 key, ``False`` otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was added here cause the docs generator tool wasnt seeing the is_valid_ed25519_pubkey function (didnt exist on utils.rs).
took a look and noticed that it was because it was kind of binded (idk what the word is) with the are_bytes_valid_ed25519_pubkey function in the wallet.rs.


updated docstrings for the Bittensor Wallet SDK to enable docs generation.