Offer to code review this repo's Bash #208
Comments
|
Sure, you already helped in previous pull requests and I would be happy to see more suggestions |
ingydotnet
added a commit
to ingydotnet/os-autoinst-scripts
that referenced
this issue
Jan 26, 2023
aka - Wrap some long lines in README.md This is just a benign first commit so that I can create a pull request for this repository. Per os-autoinst#208 I wish to code review the Bash content and make suggestions on possible improvements. I haven't done this before so I'm not sure of the best way to go about it. I'm thinking to use this pull request, where I will make a commit for each suggestion with a couple of code changes to demonstrate the objective. Then we can can discuss it. I don't expect the PR to be merged, but you might end up cherry-picking some commits in it. Let me know if this approach works for you and I'll start adding commits soon after.
|
Great. I think this will be fun and I'm sure I'll end up learning a lot as well. I still am constantly learning new/better ways to do things with Bash. @okurz I started #209 with a dumb commit. I think making each suggestion a commit of just a couple lines, but a full explanation in the commit message might be a decent way to do this. If you agree then please say so in a comment or a 👍 in #209 and I'll get started. :) |
|
Yep, good approach |
ingydotnet
added a commit
to ingydotnet/os-autoinst-scripts
that referenced
this issue
Jan 29, 2023
aka - Wrap some long lines in README.md This is just a benign first commit so that I can create a pull request for this repository. Per os-autoinst#208 I wish to code review the Bash content and make suggestions on possible improvements. I haven't done this before so I'm not sure of the best way to go about it. I'm thinking to use this pull request, where I will make a commit for each suggestion with a couple of code changes to demonstrate the objective. Then we can can discuss it. I don't expect the PR to be merged, but you might end up cherry-picking some commits in it. Let me know if this approach works for you and I'll start adding commits soon after.
ingydotnet
added a commit
to ingydotnet/os-autoinst-scripts
that referenced
this issue
Jan 30, 2023
aka - Wrap some long lines in README.md This is just a benign first commit so that I can create a pull request for this repository. Per os-autoinst#208 I wish to code review the Bash content and make suggestions on possible improvements. I haven't done this before so I'm not sure of the best way to go about it. I'm thinking to use this pull request, where I will make a commit for each suggestion with a couple of code changes to demonstrate the objective. Then we can can discuss it. I don't expect the PR to be merged, but you might end up cherry-picking some commits in it. Let me know if this approach works for you and I'll start adding commits soon after.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I've looked over the Bash code in this repo and many things are very good, but a lot of things could be improved.
I've been using Bash for 10 years now, as seriously as I use Perl, Ruby, Python, Node etc.
I've even recently written a Bash package management system (CPAN, RubyGems, PyPI, NPM) called BPAN.
I'd like to offer a code review of this repo for you. I'll make lots of suggestions and you can choose to apply the ones you like.
Most of the suggestions would just be minor syntax refactoring. Nothing (or very little) would be a major change suggestion.
But before I do, please let me know whether or not this would be appreciated.
Cheers, Ingy
The text was updated successfully, but these errors were encountered: