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

Enable long, GNU style options RE: issue-1 #3

Merged
merged 2 commits into from
Feb 16, 2016
Merged

Enable long, GNU style options RE: issue-1 #3

merged 2 commits into from
Feb 16, 2016

Conversation

zbeekman
Copy link
Collaborator

Pending final review and CI tests, @kvz please feel free to merge

  • Enable long, GNU style options, fixes short and long argument name #1
  • ⚠️ CAVEAT A short option must be preset for every long option;
    but every short option need not have a long option
  • No BASH 4 features were added, works with bash 3 and standard sed and
    awk
  • -- is still respected as the separator between options and arguments
  • Use awk only instead of awk and sed for parsing short options
    from usage string
  • Enable errexit, nounset and pipefail at the top

-t --temp [arg] Location of tempfile. Default="/tmp/bar"
-v Enable verbose mode, print script as it is executed
-d --debug Enables debug mode
-h --help This page
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The second field is examined for leading -- if found, that is determined to be a long version of the option. N.B. All long options must have corresponding short options, but not vice-versa. This could be changed without too much work, but for now, this is more consistent with the existing code and intentions, IMO.

Copy link
Owner

Choose a reason for hiding this comment

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

A very acceptable compromise imo too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Looking good so far @zbeekman! On what OS are you testing this? I'm just worried a bit that you said standard sed. I think it's either GNU or BSD, and both are quite different, where GNU is standard on most Linux distros and BSD is present on OSX.

I suppose we should set up some kind of Travis CI environment and see if we can execute this on a few environments.

@zbeekman
Copy link
Collaborator Author

Looking good so far @zbeekman!

Thanks!

On what OS are you testing this? I'm just worried a bit that you said standard sed. I think it's either GNU or BSD, and both are quite different, where GNU is standard on most Linux distros and BSD is present on OSX.

Yes I am on OS X which uses the BSD flavored sed. However, I have only made one change/addition that uses sed which is in the diff hunk I commented on here. As per your suggestion, which I agree with, this could easily be changed to use only awk which is a little bit more forgiving across machines. I'll go ahead and implement that.

I suppose we should set up some kind of Travis CI environment and see if we can execute this on a few environments.

I agree, however, my time is severely limited at the moment, and I probably can't do anything fancier than some manual testing on OS X and RHEL 5/6.

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Much appreciated! I forked off the issue of CI into #4

@zbeekman
Copy link
Collaborator Author

This is how I suggest we proceed:

My intention is to push one or two additional commits, to finalize things here, and then once you think everything looks good, and I think everything looks good, I'll squash them down into one commit and force push them to my fork. After that, unless you have additional concerns, you can go ahead and merge. Does that work for you?

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

sounds great!

@zbeekman
Copy link
Collaborator Author

I'll also rebase, since it looks like you've pushed some travis-ci commits to master. That way, some CI tests should get run on this PR, I hope. 🍀

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Leave the failing build to me, it's expected to fail until I rerun fixtures for your changes.

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Alternatively, you could run npm install && npm run save:fixtures

@zbeekman
Copy link
Collaborator Author

Alternatively, you could run npm install && npm run save:fixtures

What does the npm run save:fixtures do? I assume fixtures are your unit tests? What does the save do?

# Bash will remember & return the highest exitcode in a chain of pipes.
# This way you can catch the error in case mysqldump fails in `mysqldump |gzip`
set -o pipefail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More robust error checking from the beginning

@zbeekman
Copy link
Collaborator Author

Travis-CI tests look good, I just need to adjust the expected output for the tests.

@zbeekman
Copy link
Collaborator Author

Alternatively, you could run npm install && npm run save:fixtures

This is the output when I run it locally, FYI.

$ npm install && npm run save:fixtures
npm WARN EPACKAGEJSON [email protected] No repository field.
npm WARN EPACKAGEJSON [email protected] No license field.

> [email protected] save:fixtures /Volumes/Sandbox/bash3boilerplate
> cross-env SAVE_FIXTURES=true npm run test

sh: cross-env: command not found

npm ERR! Darwin 14.5.0
npm ERR! argv "/usr/local/homebrew/Cellar/node/5.1.1/bin/node" "/usr/local/bin/npm" "run" "save:fixtures"
npm ERR! node v5.1.1
npm ERR! npm  v3.3.12
npm ERR! file sh
npm ERR! code ELIFECYCLE
npm ERR! errno ENOENT
npm ERR! syscall spawn
npm ERR! [email protected] save:fixtures: `cross-env SAVE_FIXTURES=true npm run test`
npm ERR! spawn ENOENT
npm ERR!
npm ERR! Failed at the [email protected] save:fixtures script 'cross-env SAVE_FIXTURES=true npm run test'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the bash3boilerplate package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     cross-env SAVE_FIXTURES=true npm run test
npm ERR! You can get their info via:
npm ERR!     npm owner ls bash3boilerplate
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Volumes/Sandbox/bash3boilerplate/npm-debug.log

npm-debug.log.txt

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Sorry about that, I only just added these things and used half a brain apparently. Added a fix for that in 7a9d116.

What does the npm run save:fixtures do?

Short of the time to implement unit tests, I decided to slap a poor man's alternative on top of this in the form of acceptance tests.

npm run save:fixtures executes the run.sh files that are in https://github.com/kvz/bash3boilerplate/tree/master/test/scenario. It saves their stdout, stderr, and exit code, replacing some environment specific string with placeholder variables in them. These are saved as fixtures and to be stored under Git. npm run test executes the files again, and compares the stdout, stderr, and exit codes. If they differ, the diff is shown and we error out.

This way, any changes to the flow or success, get noticed. And we have to check them into Git if we agree.

@zbeekman
Copy link
Collaborator Author

Other than ensuring the tests pass, and squashing/rebasing this commit, I think things look good.

Regarding testing locally, I tried rebasing on your latest pushed fixes, but I still encounter an error. Please see my comment on 63cd89a

At your leisure, please review the latest commits I pushed. I'm waiting on squash and rebase so you can see my comments on the diffs. Once you give me a "LGTM" or similar, I will squash, rebase and force push. After that we can address any local test failures, if need be.

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Okay just reviewed all of it again and it lgtm! Thanks alot for this improvement. I'll be waiting on the squashed version and merge this.

Wether you succeed with the tests or no, if you don't I'll patch things up after merging, this is my mess ;)

 - Enable long, GNU style options, fixes #1
 - *CAVEAT* A short option must be preset for every long option;
   but every short option need *not* have a long option
 - No BASH 4 features were added, works with bash 3 and standard sed and
   awk
 - `--` is still respected as the separator between options and arguments
 - Use `awk` only instead of `awk` and `sed` for parsing short options
   from usage string
 - Enable errexit, nounset and pipefail at the top
 ATTN: @kvz I think I did this correctly, but please look through the
 Travis-CI output nonetheless.
@zbeekman
Copy link
Collaborator Author

Test pass locally with your latest changes. I think I figured out how to update anticipated test results too... 🙏 hopefully Travis-CI tests will pass. I'm happy to have this merged if you are. ⚡

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Great job @zbeekman, can't thank you enough. Merging this.

I won't always have time to maintain this project so I also gave you commit access to make any changes you'd like. Looking at your work I'm not concerned that will end in a disaster :)

kvz added a commit that referenced this pull request Feb 16, 2016
Enable long, GNU style options RE: issue-1
@kvz kvz merged commit e90ad74 into kvz:master Feb 16, 2016
@zbeekman
Copy link
Collaborator Author

🙇 many thanks @kvz

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

Trying this locally on OSX @zbeekman and getting:

kvz at mbp in ~/code/bash3boilerplate on master
$ ./main.sh -d
awk: syntax error at source line 1
 context is
     >>> match($1,"^-.", <<<
awk: bailing out at source line 1
2016-02-16 20:07:19 UTC [     info] Cleaning up. Done

What awk version do you have?

Mine is

$ awk --version
awk version 20070501

@kvz
Copy link
Owner

kvz commented Feb 16, 2016

@zbeekman Addressed in df3d535

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.

short and long argument name
2 participants