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 & use consistent function definitions #1464

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

hyperupcall
Copy link
Contributor

Summary

In Bash, there are there ways to declare a function:

fn() { :; }
function fn { :; }
function fn() { :; }

This PR ensures that only the first form is used.

@hyperupcall hyperupcall requested a review from a team as a code owner January 29, 2023 01:39
scripts/checkstyle.py Outdated Show resolved Hide resolved
@jthegedus
Copy link
Contributor

How is this deciding which files to apply to? lib/asdf.fish also has a function usage. Why is that not matched?

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Jan 29, 2023

How is this deciding which files to apply to? lib/asdf.fish also has a function usage. Why is that not matched?

Only bash and sh, and bats files are matched, if files are not directly supplied as arguments to checkstyle.py. Since there are relatively few lines of Fish, I opted not to do any checking of them.

@jthegedus jthegedus changed the title chore(checkstyle): Assert a single function style fix: enforce & use consistent function definitions Jan 29, 2023
@jthegedus
Copy link
Contributor

Renaming the PR to be a fix: type as it touches code executed by users, so therefore should be considered as a change in the next release version.

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Thanks

@jthegedus jthegedus merged commit e0fd7a7 into asdf-vm:master Jan 29, 2023
@hyperupcall hyperupcall deleted the h-no-function-keyword branch January 29, 2023 03:52
@Stratus3D
Copy link
Member

Thanks @hyperupcall ! I've been wanting this for years!

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.

4 participants