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

Adding docstrings for array, assert and panic macros #6627

Merged
merged 10 commits into from
Nov 19, 2024

Conversation

Gerson2102
Copy link
Contributor

@Gerson2102 Gerson2102 commented Nov 11, 2024

What it solves?

It solves this issue #6541

Changes Made

This PR is adding the docstrings for some macros in the basecode. Specifically for assert!, panic!, array! macros.

Status

This PR still work in progress. But I would like to receive some feedback about the work so far.

Errors

I dont know why I'm getting these errors for some of the macros related to format. Look these screenshots:

image
image

In many places in the code of the macros I'm getting those weird errors.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @Gerson2102)


crates/cairo-lang-semantic/src/inline_macros/array.rs line 90 at r1 (raw file):

            "#}
            .to_string(),

remove whitespace lines, and shorten lines longer than 100 chars.

Code quote:

    fn documentation(&self) -> Option<String> {
        Some(
            indoc! {r#"
            Creates a new array containing the provided elements.

            The `array!` macro allows you to create an array by specifying a list of elements. \
            The elements are added to a new array in the order they are provided.

            # Syntax

            ```cairo
            array![element1, element2, element3, ...]
            ```

            # Returns

            An array containing the specified elements.

            # Examples

            ```cairo
            let arr = array![]; // Creates an empty array.

            let arr = array![1, 2, 3]; // Creates an array containing 1, 2, and 3.

            let x = 5;
            let y = 10;
            let arr = array![x, y, x + y]; // Creates an array containing 5, 10, and 15.
            ```

            # Notes

            - All elements must be of the same type or compatible types that can be coerced to a common type.
            - The macro internally uses `ArrayTrait::new()` to create a new array and `ArrayTrait::append()` to add each element.
            - The order of the elements in the array is the same as the order they are provided in the macro.

            "#}
            .to_string(),

@Gerson2102
Copy link
Contributor Author

Lmk what you think now sir.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @Gerson2102)


crates/cairo-lang-semantic/src/inline_macros/assert.rs line 11 at r2 (raw file):

use cairo_lang_syntax::node::ast::WrappedArgList;
use cairo_lang_syntax::node::db::SyntaxGroup;
use cairo_lang_syntax::node::{ast, TypedStablePtr, TypedSyntaxNode};

Please rebase to use the latest rust fmt version.

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @Gerson2102 and @orizi)


a discussion (no related file):

  1. Please use Reviewable for commenting

I dont know why I'm getting these errors for some of the macros related to format. Look these screenshots:

You need to import the indoc macro, add use indoc::indoc

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @Gerson2102 and @mkaput)


crates/cairo-lang-semantic/src/inline_macros/array.rs line 80 at r2 (raw file):

            - Elements are added in the order provided.
            "#}
            .to_string(),

Suggestion:

    fn documentation(&self) -> Option<String> {
        Some(
            indoc! {r#"
            Creates a new array containing the provided elements.
            The `array!` macro allows you to create an array by specifying a list of elements. \
            The elements are added to a new array in the order they are provided.

            # Syntax
            ```cairo
            array![element1, element2, element3, ...]
            ```
            # Returns
            An array containing the specified elements.

            # Examples
            ```cairo
            let arr = array![]; // Creates an empty array.
            let arr = array![1, 2, 3]; // Creates an array containing 1, 2, and 3.
            let x = 5;
            let y = 10;
            let arr = array![x, y, x + y]; // Creates an array containing 5, 10, and 15.
            ```
            # Notes
            - Elements must be of the same type or convertible to a common type.
            - Uses `ArrayTrait::new()` to create the array and `ArrayTrait::append()` to add \
            elements.
            - Elements are added in the order provided.
            "#}
            .to_string(),

@Gerson2102
Copy link
Contributor Author

I was inside of the Reviewable but I did not find a way to comment through that tool, sorry. Is my first time using it.

@Gerson2102
Copy link
Contributor Author

Previously, mkaput (Marek Kaput) wrote…
  1. Please use Reviewable for commenting

I dont know why I'm getting these errors for some of the macros related to format. Look these screenshots:

You need to import the indoc macro, add use indoc::indoc

Got it sir. Now I know how to comment through reviewable. Thanks.

@Gerson2102
Copy link
Contributor Author

crates/cairo-lang-semantic/src/inline_macros/assert.rs line 11 at r2 (raw file):

Previously, orizi wrote…

Please rebase to use the latest rust fmt version.

Done.

@Gerson2102
Copy link
Contributor Author

crates/cairo-lang-semantic/src/inline_macros/array.rs line 80 at r2 (raw file):

            - Elements are added in the order provided.
            "#}
            .to_string(),

I think I addressed what you suggested. If not, please lmk.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @Gerson2102 and @mkaput)


crates/cairo-lang-semantic/src/inline_macros/assert.rs line 11 at r2 (raw file):

Previously, Gerson2102 (Gerson) wrote…

Done.

now also run ./scripts/rust_fmt.sh before pushing.

this should revert the bad formatting changes.


crates/cairo-lang-semantic/src/inline_macros/assert.rs line 132 at r3 (raw file):

            - `condition`: A boolean expression to evaluate.
            - `format_string` (optional): A string literal for format placeholders.
            - `args` (optional): Arguments corresponding to the format placeholders in the `format_string`.

shorten or split line. (under 100 rust chars)
also elsewhere if still exists.

Code quote:

            - `args` (optional): Arguments corresponding to the format placeholders in the `format_string`.

crates/cairo-lang-semantic/src/inline_macros/assert.rs line 142 at r3 (raw file):

            // Panics with "Age must be at least 21, found 18."
            let x = -1;
            assert!(x >= 0, "Invalid value: x = {}", x);

maybe we should have an example with this style?
in panic as well.

Suggestion:

            assert!(x >= 0, "Invalid value: x = {x}");

@Gerson2102
Copy link
Contributor Author

crates/cairo-lang-semantic/src/inline_macros/assert.rs line 11 at r2 (raw file):

Previously, orizi wrote…

now also run ./scripts/rust_fmt.sh before pushing.

this should revert the bad formatting changes.

Done.

@Gerson2102
Copy link
Contributor Author

crates/cairo-lang-semantic/src/inline_macros/assert.rs line 142 at r3 (raw file):

Previously, orizi wrote…

maybe we should have an example with this style?
in panic as well.

Done.

@Gerson2102
Copy link
Contributor Author

crates/cairo-lang-semantic/src/inline_macros/assert.rs line 132 at r3 (raw file):

Previously, orizi wrote…

shorten or split line. (under 100 rust chars)
also elsewhere if still exists.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gerson2102)

@Gerson2102
Copy link
Contributor Author

I dont find the assert_eq! macro, it doesn't exist on the basecode?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @Gerson2102 and @mkaput)


crates/cairo-lang-semantic/src/inline_macros/consteval_int.rs line 69 at r5 (raw file):

        Some(
            indoc! {r#"
            Evaluates an integer expression at compile time.

State the fact or is deprecated, as can be done with normal consts.


crates/cairo-lang-semantic/src/inline_macros/consteval_int.rs line 86 at r5 (raw file):

            ```
            # Notes
            - Useful for compile-time computations when const expressions were not fully supported.

It no longer is useful.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

It is in the test plugin, and not the general plugin

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @Gerson2102 and @mkaput)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Gerson2102)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Gerson2102)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gerson2102)

@Gerson2102
Copy link
Contributor Author

I found the assert_eq! macro in cairo-lang-test-plugin. And inside of the assert.rs file is not too clear for me where to add the function. Do I add it just next to the generate_codefunction and thats it?

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Gerson2102)

@mkaput mkaput linked an issue Nov 19, 2024 that may be closed by this pull request
@orizi orizi added this pull request to the merge queue Nov 19, 2024
Merged via the queue into starkware-libs:main with commit 04ced4e Nov 19, 2024
47 checks passed
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.

Compiler: Common macros don't have docstrings
4 participants