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

Add a --emit stdout flag to scarb fmt #700

Closed
ClementWalter opened this issue Sep 25, 2023 · 7 comments
Closed

Add a --emit stdout flag to scarb fmt #700

ClementWalter opened this issue Sep 25, 2023 · 7 comments
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ClementWalter
Copy link

ClementWalter commented Sep 25, 2023

Problem

There is no way to have scarb outputs the formatted result instead of overwriting the input.

This is a problem when integrating scarb with other linters in a tool like trunk.io. See this file using cairo-format:

https://github.com/ClementWalter/kakarot/blob/7686fbf022d99ce01927eb488ad59f0a0ec7ce5d/.trunk/trunk.yaml#L30

Proposed Solution

Add, like in the previous cairo-format tool, or in rustfmt a flag --emit stdout to emit and not overwrite

Notes

No response

@github-project-automation github-project-automation bot moved this to Triage in Scarb Sep 25, 2023
@mkaput mkaput moved this from Triage to Backlog in Scarb Sep 25, 2023
@mkaput mkaput added good first issue Good for newcomers help wanted Extra attention is needed labels Sep 25, 2023
@mkaput
Copy link
Member

mkaput commented Sep 25, 2023

I'm all-in to do this the way how proposed solution looks like. I believe though that there is no such option upstream either, and one would have to also push a PR to Cairo compiler first.

I'm happy to accept PRs for this, as it's not something our team has capacity to take on immediately right now 😄

@ClementWalter
Copy link
Author

created issue in cairo starkware-libs/cairo#4134

let's see what they are up to, imho just updating the behavior of --check is the easier

@orizi
Copy link

orizi commented Sep 28, 2023

the handling of the output is done in the wrapping binary - not in the cairo library itself.
so there's no actual upstream updates required.

@Eikix
Copy link
Contributor

Eikix commented Nov 1, 2023

I'd like to take this issue if possible

the handling of the output is done in the wrapping binary - not in the cairo library itself. so there's no actual upstream updates required.

I'm not sure I understand this, what is the "wrapping binary", are you referring to a part of Scarb's system?

After talking with @ClementWalter, I propose this change:

  • Change scarb fmt --check outputting a file diff to output a new file altogether. This is in accordance with the Trunk documentation about custom linters and more generally with the rewrite norm of linting.
image

@mkaput
Copy link
Member

mkaput commented Nov 6, 2023

  • Change scarb fmt --check outputting a file diff to output a new file altogether. This is in accordance with the Trunk documentation about custom linters and more generally with the rewrite norm of linting.

I would definitely not deviate from rustfmt behaviour. It prints diffs so that the output is readable and actionable when somebody is reading CI logs. No reason to change that.

Please implement --emit stdout behaviour, like @ClementWalter suggested in the issue description.


The issue is yours 😉 Thanks ♥️♥️♥️♥️♥️

@mkaput mkaput moved this from Backlog to In Progress in Scarb Nov 6, 2023
@Eikix
Copy link
Contributor

Eikix commented Nov 6, 2023

So if I understand correctly,
I should:

  • inside scarb fmt related logic, add a flag "emit", with a single option "stdout" (for now).
  • the emit flag with the option will emit a "target" file, fully formatted. with the option stdout, it'll emit it to stdout

-> no changes are made to cairo format
-> no changes are made to --check command

@mkaput
Copy link
Member

mkaput commented Nov 6, 2023

Yep!

@maciektr maciektr moved this from In Progress to Done in Scarb Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
Archived in project
Development

No branches or pull requests

5 participants