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

Printer rotation command #174

Merged
merged 9 commits into from
Feb 26, 2022
Merged

Printer rotation command #174

merged 9 commits into from
Feb 26, 2022

Conversation

Boomaa23
Copy link
Member

Fix for #101 with regards to ocf/ocfweb#754.

@Boomaa23 Boomaa23 requested a review from dkess February 16, 2022 21:59
Copy link
Member

@dkess dkess left a comment

Choose a reason for hiding this comment

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

Nice job, thanks for working on this! Have some style nits but this is definitely the right idea.

staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
@dkess
Copy link
Member

dkess commented Feb 17, 2022

One more thing: it might later be useful to add a subcommand that lists the current printer state (i.e. tell the user which printers are in/out of service). You might want to look into argparse subparsers (https://docs.python.org/3/library/argparse.html#sub-commands) for a better way of handling this (as opposed to a conditional arg for "add"/"remove"

@Boomaa23 Boomaa23 changed the title Mod printer Printer rotation command Feb 19, 2022
@Boomaa23
Copy link
Member Author

Boomaa23 commented Feb 25, 2022

Okay so I took your changes/suggestions and now it uses a subparser, with functionality to list/add/remove. Notably however CUPS doesn't provide a CLI option for listing "non-shared" printers (i.e. hardware printers, not just the classes), so the best I could do was just print the printers that are currently active. Also the entire thing is now more modern, as per your suggestions. Thanks for your help with this @dkess!

(also I'm not entirely sure why the build is failing, I see from the logs it has something to do with whitespace (?) but idk where)

@Boomaa23 Boomaa23 requested a review from dkess February 25, 2022 19:51
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
staff/lab/mod-printer Outdated Show resolved Hide resolved
@dkess
Copy link
Member

dkess commented Feb 25, 2022

Re: the build errors, the make test will run pre-commit for you and should fix them automatically.

@Boomaa23 Boomaa23 requested a review from dkess February 26, 2022 00:55
@dkess dkess merged commit 650bff2 into master Feb 26, 2022
@Boomaa23 Boomaa23 deleted the mod-printer branch February 26, 2022 04:51
jyxzhang pushed a commit that referenced this pull request Oct 23, 2022
* Parsing for mod printer

* Implemented mod-printer functionality

* Test and bugfix mod-printer

* Update mod-printer with more modern Python functions

* Change mod-printer to subparsers

* Add list_printers mod-printer base function

* Add list subcommand to mod-printer

* Minor mod-printer doc fix

* Fix mod-printer formatting, general verbosity
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.

2 participants