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

Better shellcheck support #139

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Better shellcheck support #139

wants to merge 14 commits into from

Conversation

kins-dev
Copy link

Supports --enable=all for shellcheck https://github.com/koalaman/shellcheck/blob/master/shellcheck.1.md#options
Adds shellcheck directives to properly source files

The major change was cleaning up every variable instance to be compliant with https://github.com/koalaman/shellcheck/wiki/SC2250

Thank you for having a good test suite. It caught a lot of the corner cases I missed initially.

Basically the main problem I was having is I run at a stricter level of linting than this project. So I brought it in line with the stricter set of rules. It shouldn't have any affect on people using a looser rule set.

Supports --enable=all for optional checks
Adds shellcheck directives to properly source files
@matejak matejak added this to the 2.11.0 milestone Sep 26, 2020
Copy link
Owner

@matejak matejak 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 very much for your PR - you clearly put a lot of effort in it, and I really appreciate that you have addressed typos and refreshed the code style as well. I am in favor of this change, shellcheck compliance is a meaningful and valuable goal.

It will take me some time to make a thorough review, so I went through some of those changes and pointed things out that caught my eye. Thank you again for your time and for contributing back.

Please add your name to the AUTHORS file, and to the Changelog as a new feature.

src/argbash-1to2.m4 Outdated Show resolved Hide resolved
src/argbash-init.m4 Show resolved Hide resolved
src/argbash.m4 Show resolved Hide resolved
bin/argbash-init Show resolved Hide resolved
resources/examples/simple-parsing.m4 Outdated Show resolved Hide resolved
resources/examples/simple-standalone.sh Show resolved Hide resolved
kins-dev and others added 2 commits September 26, 2020 10:27
Co-authored-by: Matěj Týč <matej.tyc@gmail.com>
Fixed comments to pull request
Cleaned up warning during make install
Added files to gitignore
@kins-dev
Copy link
Author

Cleaned up most of the comments, also fixed a warning during make install:
argbash.rst:42: (ERROR/3) Undefined substitution referenced: "OPTION_IN_PLACE".

Added to the make file missing examples

src/argbash.m4 Outdated Show resolved Hide resolved
src/argbash.m4 Outdated Show resolved Hide resolved
src/argbash.m4 Outdated Show resolved Hide resolved
src/argbash.m4 Outdated Show resolved Hide resolved
src/argbash.m4 Outdated Show resolved Hide resolved
src/argbash.m4 Outdated Show resolved Hide resolved
src/argbash.m4 Outdated Show resolved Hide resolved
src/collectors.m4 Outdated Show resolved Hide resolved
src/stuff.m4 Outdated Show resolved Hide resolved
src/stuff.m4 Outdated Show resolved Hide resolved
@matejak
Copy link
Owner

matejak commented Oct 2, 2020

We are mostly good, I have left some remarks, then I will check out complex parts of the PR again, but I don't expect that much more issues will show up.
Thank you once again for your effort, and patience that you have applied when designing this PR. The strict shellcheck support will be a great addition to existing feature set of Argbash.

kins-dev and others added 2 commits October 3, 2020 16:25
Co-authored-by: Matěj Týč <matej.tyc@gmail.com>
src/argbash-init.m4 Outdated Show resolved Hide resolved
@matejak
Copy link
Owner

matejak commented Oct 17, 2020

Looks good, the only issue that remains is the generation of this example.

@kins-dev
Copy link
Author

kins-dev commented Jan 8, 2021

I apologize for taking so long to get back to this. I fixed up the patch file and generated the docs successfully. I think that's the last item.

@kins-dev kins-dev requested a review from matejak January 8, 2021 21:19
@kins-dev
Copy link
Author

kins-dev commented Jan 8, 2021

Fixed up the docker workflow, I think that's everything

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.

2 participants