rafritts / bunit

A unit testing framework for Shell scripts - namely Bash.
MIT License
196 stars 11 forks source link

Support function declarations without the function keyword #29

Closed lfkeitel closed 7 years ago

lfkeitel commented 7 years ago

See: http://mywiki.wooledge.org/BashPitfalls#function_foo.28.29

Using the function keyword is generally discouraged and many people never use the keyword and simply use the syntax funcName() or funcName (). Both of these styles are now supported along with the use of function. There are two new tests in UnitTestsWithNoFailures.ut One has no space between the name and parens and the other does.

The minor changes on several lines is due to trailing whitespace my editor removed.

rafritts commented 7 years ago

So I think this pull request is heading in the right direction. I think the plan is to remove all mentions of the keyword "function". Either I can do it and close this pull request, or you can if you like. Up to you.

lfkeitel commented 7 years ago

I'd be happy to do it. So you want to remove the keyword from all tests and the code itself? That should be easy enough to do.

On Jan 31, 2017 03:18, "Ryan Fritts" notifications@github.com wrote:

So I think this pull request is heading in the right direction. I think the plan is to remove all mentions of the keyword "function". Either I can do it and close this pull request, or you can if you like. Up to you.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rafritts/BashScriptTestingLibrary/pull/29#issuecomment-276312933, or mute the thread https://github.com/notifications/unsubscribe-auth/AGUCXy_9ND2RednRkCCIfgQjIvXOKF65ks5rXvxcgaJpZM4LtCNJ .

lfkeitel commented 7 years ago

The deed is done. Technically, this would be a backwards compatibility break so if you're following semver, this might warrant a major version bump. However, since almost no one uses the function keyword, it may not be a big deal. But I'll leave that up to you. If there's something you don't like about the commit let me know and I'll be happy to fix it.

jasonheecs commented 7 years ago

How about modifying the regex to include support for the function keyword as well? Sorry about the commit spam, apparently rebasing the git repo doesn't remove the commit notifications 💢 .

lfkeitel commented 7 years ago

I wasn't sure if @rafritts wanted to completely remove 'function' or just its use in the library and tests. I've added support back in the regex.

rafritts commented 7 years ago

@lfkeitel You are correct in your last comment. I was hoping to have function removed from the script and tests, but I still wanted backwards compatibility. I will test this when I get home, and will merge it if it looks good. Thanks for your PR.

rafritts commented 7 years ago

Hello guys, sorry this took so long. Ive been ridiculously busy with other things =/ @lfkeitel Could I get you to open this PR to the develop branch of this repo? Then ill merge it.