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

Install Script: Add comments for nearly every line of code #1586

Merged
merged 5 commits into from Jul 12, 2017

Conversation

@jacobsalmela
Copy link
Member

jacobsalmela commented Jul 7, 2017

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

5

This pull adds comments to nearly every line in the installer. As with the new debug script, the intent of this pull is to:

  • help the developer team determine where code improvements can be made
  • promote a culture of documentation within the code via comments
  • make Pi-hole more appealing to contributors because they will be able to better understand why the code is the way that it is
  • make Pi-hole more appealing to beginners of open source who want to contribute or are just learning about projects (such as the Raspberry Pi)
  • promote better bash coding practices such as functions that do one thing, defensive programming, exit codes, etc.

This template was created based on the work of udemy-dl.

@jacobsalmela jacobsalmela added this to the v3.2 milestone Jul 7, 2017

@jacobsalmela jacobsalmela self-assigned this Jul 7, 2017

@jacobsalmela jacobsalmela requested review from PromoFaux , dschaper , Mcat12 and DL6ER Jul 7, 2017

@@ -97,57 +125,80 @@ show_ascii_berry() {

# Compatibility
distro_check() {
# If apt-get is isntalled, then we know it's part of the Debian family

This comment has been minimized.

@FDrebin

FDrebin Jul 7, 2017

Spelling error, installed.

This comment has been minimized.

@jacobsalmela

jacobsalmela Jul 7, 2017

Author Member

Fixed, thanks.

setDHCPCD() {
# Append these lines to dhcpcd.conf to enable a static IP
# but we cab append these lines to dhcpcd.conf to enable a static IP

This comment has been minimized.

@FDrebin

FDrebin Jul 7, 2017

Spelling error, can.

This comment has been minimized.

@jacobsalmela

jacobsalmela Jul 7, 2017

Author Member

Fixed, thanks.

@jacobsalmela jacobsalmela changed the title Add comments for nearly every line of code Install Script: Add comments for nearly every line of code Jul 7, 2017

phpVer="php5"
fi
# #########################################

# Since our instal script is so large, we need several other programs to successfuly get a machine provisioned

This comment has been minimized.

@FDrebin

FDrebin Jul 10, 2017

Spelling mistake , install script.

This comment has been minimized.

@jacobsalmela

jacobsalmela Jul 10, 2017

Author Member

Fixed.

local directory="${1}"
# A local variabled for the current directory

This comment has been minimized.

@FDrebin

FDrebin Jul 10, 2017

Possible grammar mistake, variable.

This comment has been minimized.

@jacobsalmela

jacobsalmela Jul 10, 2017

Author Member

Fixed.

sed -i 's/^#log-queries/log-queries/' ${dnsmasq_pihole_01_location}
fi
}

# Clean an exiting installation to prepare for upgrade/reinstall

This comment has been minimized.

@FDrebin

FDrebin Jul 10, 2017

Spelling error, clean existing.

This comment has been minimized.

@jacobsalmela

jacobsalmela Jul 10, 2017

Author Member

Fixed.

@dschaper
Copy link
Member

dschaper left a comment

LGTM

@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jul 12, 2017

@jacobsalmela Just need to pull in the changes from the Development branch and this should be good to merge.

@@ -895,178 +1217,230 @@ install_dependent_packages() {
# amount of download traffic.
# NOTE: We may be able to use this installArray in the future to create a list of package that were
# installed by us, and remove only the installed packages, and not the entire list.
if command -v debconf-apt-progress &> /dev/null; then
if command -v debconf-apt-progress &> /dev/null;

This comment has been minimized.

@dschaper

dschaper Jul 12, 2017

Member

missing a then

@dschaper
Copy link
Member

dschaper left a comment

Good to go

@jacobsalmela jacobsalmela merged commit d328d17 into development Jul 12, 2017

5 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/pullapprove Approved by dschaper, jacobsalmela
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@jacobsalmela jacobsalmela deleted the enhance/comments-in-installer branch Jul 12, 2017

@pralor

This comment has been minimized.

Copy link

pralor commented Feb 27, 2018

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-v3-2-introduces-long-term-statistics-an-audit-log-colours-and-more/5772/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.