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

[Feature Request] Implement ./tools/devtool fixstyle #4747

Closed
3 of 5 tasks
seafoodfry opened this issue Aug 23, 2024 · 2 comments
Closed
3 of 5 tasks

[Feature Request] Implement ./tools/devtool fixstyle #4747

seafoodfry opened this issue Aug 23, 2024 · 2 comments
Labels
Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later

Comments

@seafoodfry
Copy link
Contributor

seafoodfry commented Aug 23, 2024

Feature Request

While working on some documentation, I discovered that the pre-commit script relies on some python dependencies,

firecracker/pre-commit

Lines 32 to 39 in 45a1a1b

if [ "$extension" = "py" ]; then
# Apply formatters for this file
black $i
isort $i
fi
if [ "$extension" = "md" ]; then
mdformat $i
fi

I'm opening this ticket to discuss whether there'd be interest in creating a ./tools/devtool fixstyle to explicitly fix style errors and have this be used in the pre-commit.

Describe the desired solution

A possible solid solution that was offered in #4744

  • create ./tools/devtool fixstyle to run mdformat, isort, black, etc.
  • maybe have the pre-commit rely on ./tools/devtool fixstyle

Describe possible alternatives

Short term, I proposed documenting how to install mdformat in #4744 .
TL;DR create a python vitualenvironment and manually install the additional deps.

Additional context

Checks

  • Have you searched the Firecracker Issues database for similar requests?
  • Have you read all the existing relevant Firecracker documentation?
  • Have you read and understood Firecracker's core tenets?
@seafoodfry seafoodfry changed the title [Feature Request] Make pre-commit rely on ./tools/devtool [Feature Request] Implement ./tools/devtool fixstyle Aug 27, 2024
@zulinx86 zulinx86 added Good first issue Indicates a good issue for first-time contributors Status: Parked Indicates that an issues or pull request will be revisited later Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` labels Sep 2, 2024
@zulinx86
Copy link
Contributor

zulinx86 commented Sep 2, 2024

@seafoodfry I branched out the issue of KVM assumption as issue #4768. Thanks.

@roypat
Copy link
Contributor

roypat commented Nov 13, 2024

Hey @seafoodfry, I realized today that we actually already have this command! It's ./tools/devtool fmt :)

@roypat roypat closed this as completed Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later
Projects
None yet
Development

No branches or pull requests

3 participants