Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

added MAC custom info #2464

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

Conversation

thomastubby
Copy link

Description

Only fill in the fields below if relevant.

Features

added function that will pull and display MAC address, mentioned in issue #2454

Issues

TODO

added function that will pull and display MAC address
added function that will pull and display MAC address
@@ -3923,6 +3923,11 @@ get_public_ip() {
fi
}

get_hwaddr() {
hwaddr="$(ip route get 1 | awk '{print $5}')"
hwaddr="$(ip link show dev "$hwaddr" | awk '/ether/ {print $2}')"
Copy link

Choose a reason for hiding this comment

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

A random comment/nit.

You can nest $() blocks, eg:

hwaddr="$(ip link show dev "$(ip route get 1 | awk '{print $5}')" | awk '/ether/ {print $2}')"

This code surprised me a bit. My initial thought was that you would override the previous value.

Copy link
Author

Choose a reason for hiding this comment

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

I have followed previously set conventions for this project. Please see the 'get_local_ip' function.

Copy link

Choose a reason for hiding this comment

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

They do different things there. They do bashisms to remove some parts of the string. When they do something similar to what you are doing, they do this:

interface="$(route get 1 | awk -F': ' '/interface/ {printf $2; exit}')"
local_ip="$(ipconfig getifaddr "$interface")"

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I could make different variables but why would I when this works? Its easy to read and it works well enough. I do appreciate the insights though and will try to incorporate them in future feature implementations.

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