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

chore(fmt): fix emit comments #3406

Merged
merged 7 commits into from Nov 8, 2023
Merged

chore(fmt): fix emit comments #3406

merged 7 commits into from Nov 8, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 2, 2023

Description

Problem

pre

fn hello() {
    1; // 42
     2
}

post:

fn hello() {
    1; 
    // 42
    2
}

pre

// lalalalalalalal
contract Uniswap {}

post:

// lalalalalalalal contract Uniswap {}

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@ghost ghost changed the title chore: fix emit comments chore(fmt): fix emit comments Nov 2, 2023
@kevaundray
Copy link
Contributor

can you write a description to say what is being fixed here and or open up an issue?

@ghost ghost marked this pull request as draft November 3, 2023 04:23
@kevaundray
Copy link
Contributor

post: // lalalalalalalal contract Uniswap {}

Is this correct? It comments out the contract uniswap part

@kevaundray
Copy link
Contributor

Aht the test is not actually doing that though

@jfecher
Copy link
Contributor

jfecher commented Nov 3, 2023

fn hello() {
    1; // 42
     2
}

post:

fn hello() {
    1; 
    // 42
    2
}

Is this what we want? I'd think we'd always want to keep the comment on the same line unless it's prohibitively long at least:

fn hello() {
    1; // 42
    2
}

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

The description of this PR is misleading/incorrect but the expected tests look good to me.

@ghost
Copy link
Author

ghost commented Nov 3, 2023

I wrote in the problems section, this is how it was formatted before. but found other problems - moved them to draft

@ghost ghost marked this pull request as ready for review November 6, 2023 16:01
@ghost ghost requested a review from jfecher November 7, 2023 11:43
@kevaundray kevaundray enabled auto-merge November 8, 2023 12:13
@kevaundray kevaundray added this pull request to the merge queue Nov 8, 2023
Merged via the queue into noir-lang:master with commit 79d93e6 Nov 8, 2023
33 checks passed
TomAFrench pushed a commit that referenced this pull request Nov 14, 2023
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.

2 participants