Skip to content

Conversation

@Alex6323
Copy link
Contributor

@Alex6323 Alex6323 commented Oct 16, 2025

This PR renames some public API because I hope this will make it a bit easier for the user to tag/name/label results from a command, and later reference them again in follow-up commands. By no means I think this PR has to be merged. It's here to discuss whether we want to keep the old naming or whether we pick something else. IMHO the current naming is a bit confusing and favors conciseness, the new names are a bit less concise but I hope easier to understand for the user. I also added more docs to the methods in question to foster this even more.

Copy link
Contributor

@DaughterOfMars DaughterOfMars left a comment

Choose a reason for hiding this comment

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

Sorry, but I really hate this. I'm not opposed to renaming them necessarily but I think the work of explaining their usage is up to the docs no matter what and I'd rather they be shorter rather than bloating the code with long identifiers.

@Alex6323
Copy link
Contributor Author

Alex6323 commented Oct 20, 2025

Sorry, but I really hate this. I'm not opposed to renaming them necessarily but I think the work of explaining their usage is up to the docs no matter what and I'd rather they be shorter rather than bloating the code with long identifiers.

fair enough. I understand that you chose those names deliberately, but I still think that the naming is not very intuitive (even though it might be more practical if used often) and requires the user to study docs first in order to understand how the builder works. I my opinion we should reduce the entry effort for people as much as possible, and I am just wondering if that is "consice but a bit too generic names" like res and name are IMHO or "longer, more explicit names".

If you - for a second - forget, that you wrote this, would you immediatedly get that name and res are connected? Some people might also wonder, what do I name exactly? res can be confused with response. If it's not possible to understand an API without docs, then it is how it is, but I think this is not necessarily the case here. We can make an effort to make this clearer without having to consult the docs or the examples first.

Another problem with name is that you can actually provide mulitple "names" and not just a single.

Why do I think result_refs/result_ref is a good name?

  1. Because we have methods like move_call, arguments, type_arguments that represent the components of a function signature, so that there's also a method result_ is not unexpected.
  2. The connection between creating and using one is clearer (intellisense friendly)
  3. It more clearly states that it creates/uses a reference to a MoveCall result.

/// .gas(coin)
/// .gas_budget(1000000000);
/// ```
pub fn result_refs(&mut self, name: impl NamedResults) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

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

I think name_res() would also make it clearer than just name() and then res() should still be fine to reference a result

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 agree, but I don't think there's any need to have shortened names here.

  1. If a user of the api finds the method name too verbose s/he can always rename to res or whatever in the import.
  2. The user will hardly require all API that the builder provides (i.e. all 33 examples), but only a few of them tailored to their need. Hence, long names aren't really an issue, hence if I follow your naming suggestion, I would just call it name_result() on the buider, and use_result() (or whatever) as the imported method.

@Alex6323 Alex6323 changed the base branch from sdk-bindings to develop November 3, 2025 14:59
@Alex6323 Alex6323 requested a review from a team as a code owner November 3, 2025 14:59
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.

4 participants