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

Change token names to represent the character instead of the operation. #3947

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gilbens-starkware
Copy link
Contributor

@gilbens-starkware gilbens-starkware commented Aug 21, 2023

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.

Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @yuvalsw)

a discussion (no related file):
I'm mpmostly ok with this - but blocking for a team discussion.


Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware)


crates/cairo-lang-formatter/src/node_properties.rs line 93 at r1 (raw file):

                true
            }
            SyntaxKind::TokenMinus | SyntaxKind::TokenStar => {

consider asterisk

Code quote:

TokenStar

crates/cairo-lang-syntax/src/node/kind.rs line 183 at r1 (raw file):

    TokenUse,
    TerminalUse,
    TokenAnd,

consider ampersand (also for AndAnd)


crates/cairo-lang-syntax/src/node/kind.rs line 247 at r1 (raw file):

    TokenBang,
    TerminalBang,
    TokenBitNot,

consider Tilde

Code quote:

nBitNot,

crates/cairo-lang-syntax/src/node/kind.rs line 249 at r1 (raw file):

    TokenBitNot,
    TerminalBitNot,
    TokenOr,

consider pipe (also for OrOr)


crates/cairo-lang-syntax/src/node/kind.rs line 257 at r1 (raw file):

    TokenPlusEq,
    TerminalPlusEq,
    TokenQuestionMark,

consider just "Question"


crates/cairo-lang-syntax/src/node/kind.rs line 269 at r1 (raw file):

    TokenUnderscore,
    TerminalUnderscore,
    TokenXor,

consider Caret

Code quote:

enXor,

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware 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: 0 of 30 files reviewed, 7 unresolved discussions (waiting on @orizi and @yuvalsw)

a discussion (no related file):

Previously, orizi wrote…

I'm mpmostly ok with this - but blocking for a team discussion.

Reopened as we discussed it.



crates/cairo-lang-formatter/src/node_properties.rs line 93 at r1 (raw file):

Previously, yuvalsw wrote…

consider asterisk

I think I prefer star


crates/cairo-lang-syntax/src/node/kind.rs line 183 at r1 (raw file):

Previously, yuvalsw wrote…

consider ampersand (also for AndAnd)

Not sure about it, especially because TokenAmpersandAmpersand is really long.


crates/cairo-lang-syntax/src/node/kind.rs line 247 at r1 (raw file):

Previously, yuvalsw wrote…

consider Tilde

Done.


crates/cairo-lang-syntax/src/node/kind.rs line 249 at r1 (raw file):

Previously, yuvalsw wrote…

consider pipe (also for OrOr)

Done.


crates/cairo-lang-syntax/src/node/kind.rs line 257 at r1 (raw file):

Previously, yuvalsw wrote…

consider just "Question"

Not sure about it, @orizi wdyt?


crates/cairo-lang-syntax/src/node/kind.rs line 269 at r1 (raw file):

Previously, yuvalsw wrote…

consider Caret

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.

Reviewed 30 of 30 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yuvalsw)

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Changed to be mostly compatible with https://doc.rust-lang.org/reference/tokens.html#punctuation

Reviewable status: 16 of 31 files reviewed, 7 unresolved discussions (waiting on @orizi and @yuvalsw)

Copy link
Contributor Author

@gilbens-starkware gilbens-starkware 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: 16 of 31 files reviewed, 7 unresolved discussions (waiting on @orizi and @yuvalsw)


crates/cairo-lang-syntax/src/node/kind.rs line 249 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Done.

Reverted to be compatible with And.

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 15 of 15 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @yuvalsw)

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 30 files at r2, 15 of 15 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)

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 13 of 13 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yuvalsw)

Copy link
Contributor

@yuvalsw yuvalsw 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: all files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)


crates/cairo-lang-parser/src/lexer.rs line 462 at r3 (raw file):

        TokenKind::Or => SyntaxKind::TerminalOr,
        TokenKind::OrOr => SyntaxKind::TerminalOrOr,
        TokenKind::Xor => SyntaxKind::TerminalCaret,

please also rename lexer tokens:
Xor
Not
BitNot
Mul
MulEq
Div
DivEq
Mod
ModEq
MatchArrow

@gilbens-starkware gilbens-starkware force-pushed the gil/terminal_names branch 2 times, most recently from 247d828 to 052a9c5 Compare December 18, 2023 09:40
Copy link
Contributor Author

@gilbens-starkware gilbens-starkware 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: 28 of 34 files reviewed, 4 unresolved discussions (waiting on @orizi and @yuvalsw)


crates/cairo-lang-parser/src/lexer.rs line 462 at r3 (raw file):

Previously, yuvalsw wrote…

please also rename lexer tokens:
Xor
Not
BitNot
Mul
MulEq
Div
DivEq
Mod
ModEq
MatchArrow

Done.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 13 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)

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 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware)

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