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

[move reference] Review new docs #35

Merged
merged 6 commits into from
Apr 3, 2024
Merged

Conversation

tnowacki
Copy link
Contributor

  • Fixed prettier
  • Fixed formatting
  • Fixed small issues in new docs

public fun borrow_mut<T>(s: &mut Matrix<T>, i: u64, j: u64): &mut T {
borrow_mut(borrow_mut(s.v, i), j)
public fun borrow_mut<T>(s: &mut Matrix<T>, i: u64, j: u64): &mut T {
vector::borrow_mut(vector::borrow_mut(&mut s.v, i), j)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
vector::borrow_mut(vector::borrow_mut(&mut s.v, i), j)
&mut s.v[i][j]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmmm, maybe? I was avoiding having to use index syntax to explain index syntax. Thoughts?

public fun borrow<T>(s: &Matrix<T>, i: u64, j: u64): &T {
borrow(borrow(s.v, i), j)
public fun borrow<T>(s: &Matrix<T>, i: u64, j: u64): &T {
vector::borrow(vector::borrow(&s.v, i), j)
Copy link
Contributor

@cgswords cgswords Mar 29, 2024

Choose a reason for hiding this comment

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

Is this better? It is more-likely what someone would write.

Suggested change
vector::borrow(vector::borrow(&s.v, i), j)
&s.v[i][j]

default: Value
): &mut Value {
if (contains(input, *key)) {
if (contains(input, key)) {
Copy link
Contributor

@cgswords cgswords Mar 29, 2024

Choose a reason for hiding this comment

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

Can we remove copy on Key if we write this as

Suggested change
if (contains(input, key)) {
if (contains(input, &key)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could... but it would be annoying generally lol.

Take a look at the table API, all of the usages of Key are by-value because we lack life-time annotations. So the result would borrow both from the key and the table itself. The key is problematic particularly because the reference might be local

Comment on lines 155 to 158
Most of the sections in the package manifest are self explanatory, but named addresses can be a bit
difficult to understand so we examine them in more detail in
[Named Addresses During Compilation](#named-addresses-during-compilation), but before that we'll
first take a look at the `Move.lock` file and what it contains.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this is a confusion transition. I would consider swapping the two sections (since the lockfile doesn't appear to be mentioned in the address discussion) and rewrite this as:

Most of the sections in the package manifest should be self explanatory, with the exception of named addresses which are explained in the
next section. After being built, the Move.toml and configuration settings are stored in the Move.lock file.

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'll give it a shot!

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this a lot more than the current layout, so 👍🏻 from me on moving this section around.

reference/src/packages.md Outdated Show resolved Hide resolved
Comment on lines 374 to 376
`sui move <command> <command_flags>`. Unless a particular path is provided, all package commands
will run in the current working directory. The full list of commands and flags for the Move CLI can
be found by running `sui move --help`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little unclear, because, e.g., sui move test will work at any directory in a package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea this is a great callout -- this should be something like (or similar)

... Unless a particular path is provided, all package commands will run in the current enclosing Move package. ....

reference/src/packages.md Outdated Show resolved Hide resolved
Comment on lines 237 to 238
With that, let's now turn to the compilation process and how named addresses are resolved, and how
to use them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will need to change this if we do the reordering

@tnowacki tnowacki merged commit 87fa51c into MystenLabs:main Apr 3, 2024
1 check passed
@tnowacki tnowacki deleted the review branch April 3, 2024 16:52
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