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

Format Bun Fallback #997

Merged
merged 6 commits into from
Jun 27, 2024
Merged

Format Bun Fallback #997

merged 6 commits into from
Jun 27, 2024

Conversation

azaleacolburn
Copy link
Member

@azaleacolburn azaleacolburn commented Jun 26, 2024

Description

Previously, trying to run bun run format, used sub commands that ran with npm and npx. People who couldn't use npm or npmx weren't able to successfully run these scripts with bun. Now, if the npm and npmx sub commands fail, they will fall back on running them with bun and bun x respectively.

Objectives

  • Change scripts to fallback on bun

Testing Done

  • Verified that it doesn't break anything for users who can use both npm and bun, by formatting the first commit
  • Run for someone who can use bun but can't use npm, to verify that it works

No JIRA Issue

@azaleacolburn azaleacolburn requested a review from a team as a code owner June 26, 2024 20:41
@azaleacolburn azaleacolburn requested review from HunterBarclay and PepperLola and removed request for a team June 26, 2024 20:41
@azaleacolburn azaleacolburn self-assigned this Jun 26, 2024
@BrandonPacewic BrandonPacewic added the formatting Changes related to the formatting or auto-formatting of source code files. label Jun 26, 2024
@HunterBarclay
Copy link
Member

@a-crowell I think you're the only one with only bun atm. Could you try these functions out and see how it behaves on your machine?

Copy link
Collaborator

@a-crowell a-crowell left a comment

Choose a reason for hiding this comment

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

Haha works great! Thanks for this, I appreciate it.

printWidth: 120,
overrides: [
{
files: "*.json",
Copy link
Member

Choose a reason for hiding this comment

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

Why the override? Not complaining, I just don't understand. Is 2 spaces standard for JSON?

Copy link
Member Author

@azaleacolburn azaleacolburn Jun 27, 2024

Choose a reason for hiding this comment

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

I was told my @BrandonPacewic that is was. I believe it's because there's so much deep nesting / indentation in json, two spaces makes it more readable.

Copy link
Member

Choose a reason for hiding this comment

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

All of our .json files up to this point have been 2 spaces. Don't think that there is a real "standard" but it's what we have been using.

Comment on lines 80 to 86
fileLocation: str = field(
default=(
os.getenv("HOME")
if platform.system() == "Windows"
else os.path.expanduser("~")
)
)
Copy link
Member

Choose a reason for hiding this comment

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

Warning

Copy link
Member Author

@azaleacolburn azaleacolburn Jun 27, 2024

Choose a reason for hiding this comment

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

Should we wait for that to be merged, then rerun the formatter on this branch?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@PepperLola PepperLola left a comment

Choose a reason for hiding this comment

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

Since Bun seems to be considerably faster than npm/npx, would it make more sense for it to be attempted before npm? That way it will default to Bun for those who have both installed (like myself).

@azaleacolburn
Copy link
Member Author

Since Bun seems to be considerably faster than npm/npx, would it make more sense for it to be attempted before npm? That way it will default to Bun for those who have both installed (like myself).

Sure, but bun is less common than npm, and I would bet that the cost of failing bun then succeeding at npm is greater than the cost of using npm. I'll make the change though, but I think it'll be slower for more people.

@PepperLola PepperLola self-requested a review June 27, 2024 17:52
Copy link
Member

@PepperLola PepperLola left a comment

Choose a reason for hiding this comment

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

Looks good.

@HunterBarclay HunterBarclay merged commit bb76c55 into dev Jun 27, 2024
11 of 12 checks passed
@HunterBarclay HunterBarclay deleted the colbura/format-bun-fallback branch June 27, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting Changes related to the formatting or auto-formatting of source code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants