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.

Sign up for GitHub

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

Jump to bottom

Moved cli frontend of the cairo linter to scarb command #1873

Merged
merged 14 commits into from
Jan 15, 2025
Merged

Moved cli frontend of the cairo linter to scarb command #1873

merged 14 commits into from
Jan 15, 2025

Conversation

wawel37
Copy link
Member

@wawel37 wawel37 commented Jan 8, 2025

No description provided.

@wawel37 wawel37 mentioned this pull request Jan 8, 2025
Draft
maciektr
maciektr reviewed Jan 8, 2025
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
scarb/src/bin/scarb/args.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/args.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/args.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
@wawel37 wawel37 marked this pull request as ready for review January 9, 2025 11:21
@wawel37 wawel37 requested a review from a team as a code owner January 9, 2025 11:21
@wawel37 wawel37 force-pushed the cairo-lint-extension branch 2 times, most recently from 38b357e to 2fa56b2 Compare January 9, 2025 11:23
@wawel37
Copy link
Member Author

wawel37 commented Jan 9, 2025

FYI, will change the cairo-toolchain-xtasks version after we merge this

@wawel37 wawel37 requested review from maciektr and mkaput January 9, 2025 12:06
maciektr
maciektr reviewed Jan 9, 2025
View reviewed changes
scarb/tests/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
mkaput
mkaput reviewed Jan 10, 2025
View reviewed changes
scarb/src/ops/compile.rs Outdated Show resolved Hide resolved
scarb/src/ops/expand.rs Outdated Show resolved Hide resolved
mkaput
mkaput requested changes Jan 10, 2025
View reviewed changes
scarb/src/bin/scarb/args.rs Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
@wawel37 wawel37 mentioned this pull request Jan 10, 2025
Open
@wawel37
Copy link
Member Author

wawel37 commented Jan 10, 2025

Moved the fixing logic back to cairo-lint-core. software-mansion/cairo-lint#179. As soon as it's merged, will upgrade the deps here in Scarb

@maciektr
Copy link
Contributor

@wawel37 Maybe we could calculate the change in cairo-lint-core and write to file here in Scarb? Wdyt?

@wawel37
Copy link
Member Author

wawel37 commented Jan 10, 2025 •
edited
Loading

@wawel37 Maybe we could calculate the change in cairo-lint-core and write to file here in Scarb? Wdyt?

TBH I've thought about it. I think both of the solutions are good. I mean, if we put the logic of mutating the files back in the Scarb, the responsibility of fixing will be separated to 2 different places but on the other hand everything that mutates files will stay within Scarb logic.

tl;dr:
I don't have a strong opinion on that. Maybe @mkaput got some opinion on that?

@mkaput
Copy link
Member

mkaput commented Jan 13, 2025

decoupling fix computation from application will be very handy for CairoLS integration, because we need to send fix patches to the langage server as text messages there

mkaput
mkaput reviewed Jan 13, 2025
View reviewed changes
scarb/Cargo.toml Outdated Show resolved Hide resolved
scarb/Cargo.toml Outdated Show resolved Hide resolved
@wawel37 wawel37 force-pushed the cairo-lint-extension branch from a654a0d to 7e6fa43 Compare January 13, 2025 10:58
@wawel37 wawel37 force-pushed the cairo-lint-extension branch from ab5647b to 8dc6ba6 Compare January 13, 2025 13:28
@wawel37 wawel37 requested a review from maciektr January 14, 2025 10:34
maciektr
maciektr reviewed Jan 14, 2025
View reviewed changes
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
scarb/src/ops/lint.rs Outdated Show resolved Hide resolved
@wawel37 wawel37 requested a review from maciektr January 14, 2025 15:15
maciektr
maciektr approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@maciektr maciektr left a comment

Choose a reason for hiding this comment

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

💪

@maciektr maciektr added this pull request to the merge queue Jan 14, 2025
@maciektr maciektr removed this pull request from the merge queue due to a manual request Jan 14, 2025
@maciektr maciektr added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 152926a Jan 15, 2025
18 checks passed
@maciektr maciektr deleted the cairo-lint-extension branch January 15, 2025 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maciektr maciektr maciektr approved these changes

@mkaput mkaput mkaput approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wawel37 @maciektr @mkaput