search

autopkg / MLBZ521-recipes

AutoPkg Recipes
MIT License
14 stars 21 forks source link

Basic python3 compatibility #5

Closed homebysix closed 4 years ago

homebysix commented 5 years ago

In order to make your processors more compatible with AutoPkg running in Python 3, I've applied a few adjustments suggested by python-modernize, pylint --py3k, and pylint in general.

More adjustments may need to be made manually later (including the use of sys.exc_clear(), which I'm not familiar with), but this should catch the low-hanging fruit right now.

gregneagle commented 5 years ago

Curious about this change: "Catch generic BaseException instead of Exception".

What is the the thinking here? Generally it's discouraged to catch Exception because it's too broad, but "fixing" that by switching to BaseException is almost certainly the wrong thing to do. It might trick the linter you are using, but BaseException is even broader than Exception, and notably catches things like Control-C and kill -SIGINT...

homebysix commented 5 years ago

To be honest, the reason is simply because pylint complains about Exception but not BaseException. I found no functional difference between the two in my testing.

It would probably be better to catch a more specific exception, but I'd argue that's out of the scope of this particular PR's goals.

gregneagle commented 5 years ago

So then I think changing Exception to BaseException makes the code worse.

It’s not a certainty that a code change that silences a pylint warning actually makes the code better — in this case the change makes the code worse.

homebysix commented 5 years ago

OK, I switched back to Exception in a854341.

homebysix commented 4 years ago

Hi @MLBZ521 - Just checking in to see whether you're comfortable merging this in, or have any questions?

Some additional processor changes will be needed for the upcoming version 2.0 of AutoPkg, and it would be nice to build on the Python 3 compatibility adjustments made in this PR.

homebysix commented 4 years ago

I just added URLGetter to MaplePatchProcessor.

I tried to do the same for XeroxPrintDriverProcessor, but it looks like Xerox's printer driver page no longer contains "macOS Common Print Driver Installer" in the source code. Might require some reworking of the regular expressions, but no need to hold up the PR for that.

homebysix commented 4 years ago

Quick note: AutoPkg 2.0 is due to be released in early February 2020, and will run in Python 3 only.

MLBZ521 commented 4 years ago

I apologize for the delay. I was out of the office for an extended period last year (September - December) on paternity leave with a stint in the NICU and also had a roof leak at home...so wasn't able to get to these unfortunately.

@homebysix Thanks for the PRs and follow ups on everything. I've tested everything up to this point and made sure to resolve any issues that were still present after your changes (for recipes still failing) as well as any issues with other recipes/processors that needed to be updated.

Currently I've been committing everything to the branch Autopkg 2.0 for now. That said, should I wait before merging everything into master until the release of 2.0....or?

homebysix commented 4 years ago

Hi @MLBZ521 - You can go ahead and merge when you're ready. All the changes made in this PR are compatible with both AutoPkg 1.x and 2.x.

MLBZ521 commented 4 years ago

@homebysix Ok cool, thanks. I'll test my changes on AutoPkg 1.x before I merge. Wasn't sure what the community was doing so thought I'd ask, don't want to break people's existing workflows.