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

[docs] Add some string-ascii and string-utf8 types to the docs #2676

Merged
merged 25 commits into from
Jun 14, 2021

Conversation

gregorycoppola
Copy link
Contributor

Fixes #2650

Changes

This PR changes the documents for the following functions: map, filter, fold, concat, as-max-len?, len, element-at, index-of.

Background

From #2650:

"When string-ascii and string-utf8 were introduced, many functions documentation in the Clarity reference were not modified to include references to these new types when applicable."

In fact, some times the examples of valid usage already include the strings, but the function documentation does not say this is allowed, e.g., "len", "element-at".

Testing

  1. I ran cargo test vm::docs

This should mostly just test that the new file compiles.

  1. I tested the new documentations that I added by creating a contract with the following print statements.

    ;;; tested for the PR
    ;;; map
    (print (map my-and 0x010203 0x010203))
    (print (map add-one-char u"hello world"))
    (print (map add-one-ascii "hello world"))
    ;;; filter
    (print (filter is-three 0x01020304030201))
    (print (filter is-vowel u"hello world"))
    (print (filter is-vowel-ascii "hello world"))
    ;;; concat
    (print (concat "concat1" "concat2"))
    (print (concat "concat1" "concat2"))
    ;;; len
    (print (len "hello world"))
    (print (len u"hello world"))
    ;;; element at
    (print (element-at u"hello world" u2))
    (print (element-at "hello world" u2))
    ;;; index of
    (print (index-of u"hello world" u"l"))
    (print (index-of "hello world" "l"))

@gregorycoppola gregorycoppola requested a review from kantai May 27, 2021 20:21
@gregorycoppola gregorycoppola self-assigned this May 27, 2021
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

These changes look great, thanks @gregorycoppola!

The only thing I would add is some more examples for the changed docs, for example, adding like

(filter buff-filter 0x00110022003300440055) ;; Returns 0x1122334455

And some similar ones for string-ascii (we probably don't need examples for both string-ascii and string-utf8).

@gregorycoppola
Copy link
Contributor Author

Now "map", "filter", "fold", "concat", "as-max-len", "len", "element-at", and "index-of", should each have "string" and "buff" examples.

@gregorycoppola
Copy link
Contributor Author

These changes look great, thanks @gregorycoppola!

The only thing I would add is some more examples for the changed docs, for example, adding like

(filter buff-filter 0x00110022003300440055) ;; Returns 0x1122334455

And some similar ones for string-ascii (we probably don't need examples for both string-ascii and string-utf8).

@gregorycoppola
Copy link
Contributor Author

I closed the comment.. not sure if I was supposed to close it or let you close it, but let's see.

@psq psq reopened this Jun 3, 2021
@psq
Copy link
Contributor

psq commented Jun 3, 2021

I closed the comment.. not sure if I was supposed to close it or let you close it, but let's see.

that closed the whole PR, not what you wanted, I'm guessing, so I reopened so this does not get lost. Sometimes, for inline comments, you can mark as resolved but not in this case.

Nice to see all these extra examples, thank you for doing this!

@gregorycoppola
Copy link
Contributor Author

@psq Thanks for reopening :)

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @gregorycoppola!

@gregorycoppola gregorycoppola requested a review from pavitthrap June 3, 2021 21:00
@gregorycoppola
Copy link
Contributor Author

Thanks @kantai .. added @pavitthrap for second review.

src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
@gregorycoppola gregorycoppola changed the title Add some string-ascii and string-utf8 types to the docs docs: Add some string-ascii and string-utf8 types to the docs Jun 4, 2021
@gregorycoppola
Copy link
Contributor Author

Thanks for the reviews @pavitthrap and @jcnelson.

I addressed and resolved some comments, but left some open.

Importantly, I left open the question of how to disjoin the various sequence forms that a function can take. In the case of 'map', 'filter' and 'fold', the resulting forms can be complex. See discussion inline where I suggest an option, but ask for feedback.

Thanks!

Various changes to the focus functions were made for clarity and consistency.
@gregorycoppola
Copy link
Contributor Author

I just pushed a version of this change that refactors the sequence arguments for readability and consistency.

See comment #2676 (comment)

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

Just had a few minor comments left! Thanks for updating the docs :)

src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
description: "The `map` function applies the function `func` to each corresponding element of the input sequences,
and outputs a _list_ of the same type containing the outputs from those function applications.
Applicable sequence types are `(list A)`, `buff`, `string-ascii` and `string-utf8`,
for which the corresponding element types are, repsectively, `A`, `(buff 1)`, `(string-ascii 1)` and `(string-utf8 1)`.
Copy link
Member

Choose a reason for hiding this comment

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

repsectively -> respectively

src/vm/docs/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple typos!

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Thanks @gregorycoppola! LGTM. Please make sure all tests pass before merging -- looks like there's a unit test failure?

@psq
Copy link
Contributor

psq commented Jun 10, 2021

After all these iterations, the final version is much better now. Thank you!

@jcnelson jcnelson changed the base branch from master to develop June 14, 2021 15:12
@gregorycoppola gregorycoppola changed the title docs: Add some string-ascii and string-utf8 types to the docs [docs] Add some string-ascii and string-utf8 types to the docs Jun 14, 2021
@gregorycoppola gregorycoppola merged commit b86576c into develop Jun 14, 2021
@kantai kantai deleted the fix/string-docs branch March 28, 2023 19:46
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Clarity reference missing string-ascii and string-utf8 references for many functions
7 participants