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

fix: enforce consistent shell redirection format #1533

Merged

Conversation

hyperupcall
Copy link
Contributor

Summary

Make redirections more consistent

@hyperupcall hyperupcall requested a review from a team as a code owner April 6, 2023 11:36
@jthegedus jthegedus requested a review from Stratus3D April 6, 2023 13:09
@jthegedus
Copy link
Contributor

I have no objection to this change. I would like to get @Stratus3D opinion.

Copy link
Member

@Stratus3D Stratus3D left a comment

Choose a reason for hiding this comment

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

I could go either way on this - I actually wasn't familiar with the &> shorthand (maybe that's obvious looking at the codebase 😛 ), but it does make the code cleaner.

@hyperupcall &> isn't POSIX is it? I assume we can only enforce this on Bash files.

@jthegedus
Copy link
Contributor

@hyperupcall &> isn't POSIX is it? I assume we can only enforce this on Bash files.

I guess if it isn't, we would just want to ensure checkstyle.py only asserts/fixes this rule for .bash files.

@hyperupcall hyperupcall force-pushed the checkstyle-consistent-redirections branch from e7ad608 to 246bbd2 Compare April 6, 2023 14:17
@hyperupcall
Copy link
Contributor Author

hyperupcall commented Apr 6, 2023

Yeah it's not POSIX, so the rule would have conflicted with ShellCheck in .sh files, good catch.

I modified the CheckStyle so this new rule only applies to our POSIX shell Bash files 😄

@jthegedus
Copy link
Contributor

so this new rule only applies to our POSIX shell files

@hyperupcall Do you mean non-POSIX? We want to apply this to .bash and NOT .sh, right?

Sorry, I am getting confused.

@hyperupcall
Copy link
Contributor Author

@jthegedus My apologies, that was me - fixed

@Stratus3D
Copy link
Member

@jthegedus you can merge whenever you are ready.

@jthegedus jthegedus changed the title fix: Make redirections consistent fix: enforce consistent shell redirection format Apr 11, 2023
@jthegedus jthegedus merged commit 1bc205e into asdf-vm:master Apr 11, 2023
10 checks passed
@hyperupcall hyperupcall deleted the checkstyle-consistent-redirections branch April 13, 2023 04:56
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

3 participants