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

Use case for Linux distro choice in install.sh #1200

Merged
merged 1 commit into from Sep 11, 2023
Merged

Use case for Linux distro choice in install.sh #1200

merged 1 commit into from Sep 11, 2023

Conversation

mentalisttraceur
Copy link
Contributor

Just a small stylistic cleanup. I was looking inside install.sh and spotted this if which can be less noisy as a case.

@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 29, 2023 11:22pm

@mentalisttraceur
Copy link
Contributor Author

mentalisttraceur commented Aug 29, 2023

[formatting notes]

I imitated the formatting used in the other two case statements in this file as much as reasonable (the only thing I did differently was add a line break between the patterns and their commands, since that on-the-same-line condensed form stops making sense when the case patterns and/or the commands are too long), but I recommend having case patterns on the same intendation level as the case keyword/block. I don't think it adds any clarity or readability to have two indent levels for every case when a single one contains the same information unambiguously.

I also kept the quoting the same, but in shell I recommend the habit/practice of using ' instead of " whenever you don't need the special interpretation/expansion that " has.

The file seems to mix tab-for-indent and two-spaces-for-indent. I used tab for the lines I touched because that seems to be the intended (far more common) one, but I left the other lines alone to keep the PR diff clean/focused.

@mentalisttraceur
Copy link
Contributor Author

I don't know why 5/6 of the PR's automated workflows are in "Expected - Waiting for status ..." state. Anyone have any ideas? Should I just push a dummy empty commit to force it to re-run or something?

Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

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

Thank you! That's much neater.

The CI wasn't running as I need to approve it

Seeing as this is your first time contributing, if you would like a holographic contributors-only Atuin sticker, then please fill out this form!

We do also have a Discord if you'd like to ask any questions, or just fancy hanging out!

@ellie
Copy link
Member

ellie commented Sep 11, 2023

I'll merge regardless of the failure, as it's unrelated to this diff. I suspect a rebase would resolve it.

@ellie ellie merged commit 2342a33 into atuinsh:main Sep 11, 2023
8 of 9 checks passed
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.

None yet

2 participants