Skip to content

Conversation

@chrisbraucker
Copy link

@chrisbraucker chrisbraucker commented Sep 11, 2023

As discussed in #148 and preliminarily implemented in #149, I adapted the code from @samba2 according to requested code style changes from @kbr, then refactored and added additional logic to make it easy to wake a host by either MAC, IP, hostname or the ID number of the internal hosts table of the fritz box.

I'm eager to hear your thoughts on this.

Should close #148 and #149

Copy link

@samba2 samba2 left a comment

Choose a reason for hiding this comment

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

When is this one raised? Reviewing on m
y phone so I might be blind😉

@chrisbraucker
Copy link
Author

umm, you only commented on the PR, not a particular line/file, so I can only guess 🤔

@kbr
Copy link
Owner

kbr commented Jul 16, 2024

Thank you very much for the pull-request and sorry for the delay. I have not checked the code by a test run so far, but by style. For the argument-parser I suggest to define separate options instead of a single complex one. I skipped the index n, because the index may change. Also the name could be ambigious, but I kept it because it can be convenient if names are unambigious. Furthermore I would suggest to not define an additional Exception but use the FritzArgumentError Exception, because the errors are argument errors. Regarding this wake_host and add_arguments could change like (untested):

def wake_host(fh, args):

    if args.device_mac_address:
        mac = args.device_mac_address

    elif args.device_ip_address:
        try:
            host = fh.get_specific_host_entry_by_ip(args.device_ip_address)
        except (FritzArgumentError, FritzLookUpError) as err:
            msg = f"Error: unknown IP {args.device_ip_address}"
            raise FritzArgumentError(msg)
        mac = host['NewMACAddress']
        
    elif args.device_name:
        device_name = args.device_name.lower()
        for entry in fh.get_generic_host_entries():
            if entry['NewHostName'].lower() == device_name:
                mac = entry['NewMACAddress']
                break
        else:
            msg = f"Error: unknown device name '{args.device_name}'"
            raise FritzArgumentError(msg)
            
    else:
        msg = f"Error: missing device specification"
        raise FritzArgumentError(msg)
        
    fh.wakeonlan_host(mac)
        

def add_arguments(parser):
    parser.add_argument(
        "--device-ip",
        dest="device_ip_address",
        default=None,
        help="ip-address of device to wake up",
    )
    parser.add_argument(
        "--device-mac",
        dest="device_mac_address",
        default=None,
        help="mac-address of device to wake up",
    )
    parser.add_argument(
        "--device-name",
        dest="device_name",
        default=None,
        help="name of device to wake up",
    )

@kbr kbr mentioned this pull request Jul 16, 2024
@chrisbraucker
Copy link
Author

Thank you for the detailed feedback, I'll adapt the code towards your suggestions as soon as my schedule allows for :)

@marklagendijk
Copy link

I'm very much interested in this. Now I understand the whole dynamic between open source projects and switching priorities and life etc. That being said, is there any chance this could be finished at some point?

@chrisbraucker
Copy link
Author

chrisbraucker commented Jun 14, 2025

So, the anniversary to my last comment is on the horizon, and thanks to @marklagendijk this PR was brought to my attention again.
I incorporated the changes requested by @kbr and updated the accompanying tests.
My base branch is at the latest head of the upstream here, and I changed the target branch to dev as suggested in the contributing guidelines.
Tests ran to completion without errors on my machine with latest nox, pytest and requests.

@kbr I do think it would make sense to add the dev dependencies like nox and pytest to the setup.py, but have not added these as it's outside of the scope of this PR.

Additionally I grouped the arguments for the CLI tool into a mutex argument group with matching description so the executing logic does not have to check for multiple passed flags.

If the short hand flags I chose for the parameters are undesired, let me know and I'll remove them again.

Let's get this merged :)

@chrisbraucker chrisbraucker changed the base branch from master to development June 14, 2025 09:57
@kbr
Copy link
Owner

kbr commented Jun 14, 2025

Thank you for caring about this again. Flags for the cli are ok (I dislike a bit the uppercase -I but don't know a better lowercase character for an ip as i is already in use) and a mutual exclusive group for the different kind of arguments is a good idea.

Regarding nox as dev-dependency: nox is designed to be installed globally. Even if it is used this way or not, I think it is best to keep nox out of the dependencies.

@chrisbraucker
Copy link
Author

Thank you for the clarification on your perspective, it sounds reasonable 👍
From my side this PR is ready to be merged. Please let me know if there are any other requirements or improvements to be made :)
If not, merge at will.

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.

Support Fritz Backed up Wake on LAN via CLI

4 participants