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: nargo info now prints information as a prettified table #2282

Merged
merged 9 commits into from
Aug 14, 2023

Conversation

Ethan-000
Copy link
Contributor

@Ethan-000 Ethan-000 commented Aug 11, 2023

Description

Problem*

Related to #2089

Summary*

run nargo info

fn main(x : Field, y : pub Field) {
    assert(x * 2 == y * 3);
}

contract Foo {
    fn double(x: Field) -> pub Field { x * 2 }
    fn triple(x: Field) -> pub Field { x * 3 }
    internal fn quadruple(x: Field) -> pub Field { x * 4 }
    open internal fn skibbidy(x: Field) -> pub Field { x * 5 }
}
+----------+-----------+------------------------+--------------+----------------------+
| Contract | Function  | Language               | ACIR Opcodes | Backend Circuit Size |
+----------+-----------+------------------------+--------------+----------------------+
| Foo      | quadruple | PLONKCSat { width: 3 } | 1            | 2                    |
+----------+-----------+------------------------+--------------+----------------------+
| Foo      | double    | PLONKCSat { width: 3 } | 1            | 2                    |
+----------+-----------+------------------------+--------------+----------------------+
| Foo      | triple    | PLONKCSat { width: 3 } | 1            | 2                    |
+----------+-----------+------------------------+--------------+----------------------+
| Foo      | skibbidy  | PLONKCSat { width: 3 } | 1            | 1                    |
+----------+-----------+------------------------+--------------+----------------------+

Additional Context

PR Checklist*

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

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Table looks good but you've made the compile_success integration tests much less strict.

crates/nargo_cli/build.rs Outdated Show resolved Hide resolved
@kevaundray kevaundray requested a review from TomAFrench August 11, 2023 20:52
@Ethan-000 Ethan-000 mentioned this pull request Aug 13, 2023
2 tasks
@TomAFrench TomAFrench enabled auto-merge August 14, 2023 08:56
@TomAFrench TomAFrench added this pull request to the merge queue Aug 14, 2023
@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Aug 14, 2023
@TomAFrench
Copy link
Member

Currently if you use this in a workspace you get the output

+---------+------------------------+--------------+----------------------+
| Package | Language               | ACIR Opcodes | Backend Circuit Size |
+---------+------------------------+--------------+----------------------+
| a       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+
+---------+------------------------+--------------+----------------------+
| Package | Language               | ACIR Opcodes | Backend Circuit Size |
+---------+------------------------+--------------+----------------------+
| b       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+

Ideally this would be merged as so:

+---------+------------------------+--------------+----------------------+
| Package | Language               | ACIR Opcodes | Backend Circuit Size |
+---------+------------------------+--------------+----------------------+
| a       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+
| b       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+

Similarly we should collect contracts across different packages and collate them into the same table.

@TomAFrench TomAFrench self-requested a review August 14, 2023 09:00
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

See comment

@kevaundray
Copy link
Contributor

Currently if you use this in a workspace you get the output

+---------+------------------------+--------------+----------------------+
| Package | Language               | ACIR Opcodes | Backend Circuit Size |
+---------+------------------------+--------------+----------------------+
| a       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+
+---------+------------------------+--------------+----------------------+
| Package | Language               | ACIR Opcodes | Backend Circuit Size |
+---------+------------------------+--------------+----------------------+
| b       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+

Ideally this would be merged as so:

+---------+------------------------+--------------+----------------------+
| Package | Language               | ACIR Opcodes | Backend Circuit Size |
+---------+------------------------+--------------+----------------------+
| a       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+
| b       | PLONKCSat { width: 3 } | 5            | 5                    |
+---------+------------------------+--------------+----------------------+

Similarly we should collect contracts across different packages and collate them into the same table.

Why do we want this to be merged?

@Ethan-000
Copy link
Contributor Author

Ethan-000 commented Aug 14, 2023

I suppose depends on whether in a workspace, can different packages be compiled to different languages?

@Ethan-000
Copy link
Contributor Author

is it better to also add some coloring to this table

@Ethan-000
Copy link
Contributor Author

Ethan-000 commented Aug 14, 2023

now print something like this 😂
with color

@Ethan-000
Copy link
Contributor Author

(i can remove the colour if it is not prefered)

@kevaundray
Copy link
Contributor

kevaundray commented Aug 14, 2023

now print something like this 😂
with color

How does this print for contracts across different packages (per Tom's comment)?

@kevaundray
Copy link
Contributor

I suppose depends on whether in a workspace, can different packages be compiled to different languages?

I think its a matter of preference, so rationale for preferences would be good

@Ethan-000
Copy link
Contributor Author

Ethan-000 commented Aug 14, 2023

now print something like this 😂
with color

How does this print for contracts across different packages (per Tom's comment)?

workspace packages

workspace color

workspace contract

workspace contract

@Ethan-000
Copy link
Contributor Author

if there are two contracts and one package in workspace

1

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM

@kevaundray kevaundray added this pull request to the merge queue Aug 14, 2023
@kevaundray kevaundray removed this pull request from the merge queue due to a manual request Aug 14, 2023
@TomAFrench TomAFrench changed the title chore: Print info as a table chore: Print output from nargo info as a table Aug 14, 2023
@kevaundray kevaundray changed the title chore: Print output from nargo info as a table chore: info command now prints information as a prettified table Aug 14, 2023
@kevaundray kevaundray changed the title chore: info command now prints information as a prettified table chore: Info command now prints information as a prettified table Aug 14, 2023
@TomAFrench TomAFrench changed the title chore: Info command now prints information as a prettified table chore: nargo info now prints information as a prettified table Aug 14, 2023
@kevaundray
Copy link
Contributor

oof sorry @TomAFrench seems there was a race condition with changing the PR title 😁

@kevaundray kevaundray added this pull request to the merge queue Aug 14, 2023
Merged via the queue into master with commit 053662a Aug 14, 2023
@kevaundray kevaundray deleted the e/pretty_print branch August 14, 2023 12:30
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master:
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master: (23 commits)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
  fix: Fix assignment when both `mut` and `&mut` are used (#2264)
  feat: Add `assert_constant` (#2242)
  feat(nargo): Add support for contracts in `nargo check` (#2267)
  chore(ci): Name wasm job more clearly (#2269)
  chore(ci): Create cache key with consideration to target (#2273)
  chore(ci): Run publish workflow against PRs (#2268)
  ...
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master: (26 commits)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  ...
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