Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

First fixes for shellcheck#76

Merged
pbrisbin merged 3 commits intopbrisbin:masterfrom
aragon999:feature/code-reformat
Sep 10, 2019
Merged

First fixes for shellcheck#76
pbrisbin merged 3 commits intopbrisbin:masterfrom
aragon999:feature/code-reformat

Conversation

@aragon999
Copy link
Contributor

So I worked a bit through the output of shellcheck. If you see anything problematic, or something you would like to solve in a different way, let me know.

The following two things are still open (I also wrote a short note in the code):
https://www.shellcheck.net/wiki/SC2076
https://www.shellcheck.net/wiki/SC2086

The $makepkg_options I would create as array and just add all the options.
About the other issue I am not really sure why this was done like that. I will look into that tomorrow.

Copy link
Owner

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

I'm good with this as an incremental improvement. I left a few comments about the TODOs.

@aragon999 aragon999 force-pushed the feature/code-reformat branch from 71531b2 to 4cd202f Compare September 8, 2019 17:52
@aragon999
Copy link
Contributor Author

I added one test and modified the build test, such that the makepkg options are also checked. I think the changes are finished so far. Do you see something else which should be changed?

@aragon999 aragon999 marked this pull request as ready for review September 8, 2019 17:59
Copy link
Owner

@pbrisbin pbrisbin left a comment

Choose a reason for hiding this comment

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

Let some non-blocking comments. Let me know what you think, if you agree and would like to address them before I merge.

@aragon999
Copy link
Contributor Author

Hey, I will address the comments and fix them in this PR, probably today :-)

@aragon999 aragon999 force-pushed the feature/code-reformat branch from 4cd202f to 7f00f12 Compare September 10, 2019 10:58
@pbrisbin pbrisbin merged commit 50974a5 into pbrisbin:master Sep 10, 2019
@pbrisbin
Copy link
Owner

Thanks!

@aragon999 aragon999 deleted the feature/code-reformat branch September 10, 2019 14:33
@aragon999
Copy link
Contributor Author

Thank you again for merging.

One little question, when (or which features other) do you plan for the next release? Can I help with something?

@pbrisbin
Copy link
Owner

One little question, when (or which features other) do you plan for the next release? Can I help with something?

Releasing isn't a big deal, so I'm happy to do it whenever. Basically any time a PR lands that fixes a bug or adds a feature, I'll release shortly after. I probably won't release for this PR alone, since it's not user-facing.

Pushing along other open PRs or triaging and/or addressing open Issues would be my first preference for helping out. After that, I'm happy to review and hopefully release whatever you're interested in doing!

@aragon999
Copy link
Contributor Author

Releasing isn't a big deal, so I'm happy to do it whenever. Basically any time a PR lands that fixes a bug or adds a feature, I'll release shortly after. I probably won't release for this PR alone, since it's not user-facing.

I see. Yes I was not asking because of this PR, but the other one (#70) with the gawk issue. Since I currently have aurget not patched on one system, and notice it there every day :-)
It might probably be good, to release an update for this problem, see e.g. the following output:

$ aurget -Syu     
:: Starting AUR upgrade...
awk: cmd:4: Warning: regexp escape sequence `\"' is not a known regexp operator
:: Searching AUR...
awk: cmd:4: Warning: regexp escape sequence `\"' is not a known regexp operator

Targets (3): adminer-4.7.3-1 intellij-idea-ultimate-edition-2019.2.2-1 intellij-idea-ultimate-edition-jre-2019.2.2-1

It is somewhat harder to quickly scan the output.

@pbrisbin
Copy link
Owner

pbrisbin commented Sep 11, 2019 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants