CGCookie / blender-addon-updater

A module for enabling users to check for add-on updates and install new versions directly from Blender.
GNU General Public License v3.0
247 stars 42 forks source link

Formatting pep8 #73

Closed neomonkeus closed 3 years ago

neomonkeus commented 3 years ago

Apply pep8 formatting to code base

Note: Made a backwards in compatible change by renaming updater_intrval_minutes to updater_interval_minutes.

TheDuckCow commented 3 years ago

Sorry I didn't get back to you sooner, been tied up! And thanks for opening this as a new PR. I just want to make sure I do a thorough check before merging it in, while also trying to figure out fixes for Blender 2.93. Will try to get to it this week!

neomonkeus commented 3 years ago

Yeah was doing some of my own tidy up of my repos, so figured it was less work on you if I just recreated the PR using the right source branch this time. And no worries, absolute no rush especially at the expense of testing.

I was also playing with 2.93 and got the errors mentioned. There are a few reversal of pep conventions when moving to python 3.9, when it comes to string comparisons

SyntaxWarning: "is not" with a literal. Did you mean "!="?
  if self.error is not "":

/Users/monkey/Library/Application Support/Blender/2.93/scripts/addons/io_scene_niftools/addon_updater_ops.py:1105:
SyntaxWarning: "is not" with a literal. Did you mean "!="?
  elif last_check is not "" and last_check is not None:

But this PR is big enough as is.

TheDuckCow commented 3 years ago

FYI I've started to review this. To make it clean, I'm going to aim to review (maybe push some changes back up if needed), and merge as is before trying to tackle any 2.93 update changes, just to isolate that out. I'll comment back soon as I summarize any of my thoughts or counter proposed changes. Thanks again for contributing here!

Noted on pep8 reversals, will consider this. I think when comparing string literals to empty strings, the new way is better as it's less ambiguous (where is not I typically now use more for comparing to None if I don't explicitly say that).

Side note, curious to hear if the little addon_updater_test.py works for you if you try it (designed to run either by command line, or as a script inside the blender interface if loaded from file directly). I'm slowly trying to build up a standard for testing blender addons to reduce the need for doing all the manual spot checks.

neomonkeus commented 3 years ago

FYI I've started to review this. To make it clean, I'm going to aim to review (maybe push some changes back up if needed), and merge as is before trying to tackle any 2.93 update changes, just to isolate that out. I'll comment back soon as I summarize any of my thoughts or counter proposed changes. Thanks again for contributing here!

Noted on pep8 reversals, will consider this. I think when comparing string literals to empty strings, the new way is better as it's less ambiguous (where is not I typically now use more for comparing to None if I don't explicitly say that).

Side note, curious to hear if the little addon_updater_test.py works for you if you try it (designed to run either by command line, or as a script inside the blender interface if loaded from file directly). I'm slowly trying to build up a standard for testing blender addons to reduce the need for doing all the manual spot checks.

I attempted to the get tests running, but they unfortunately didn't get very far with it. I tried both to run it via the console and via command-line. TBH, I am not really surprised it is unable to find it due to the way I have it currently embedded the addon updater within my addon structure. I do have other dependencies which I "install" via pip into the build. Going to play around with turning it into a standalone module, which I think would really help generally decouple things as reduce my copy pasting between updater repo & upstream dependant.

TheDuckCow commented 3 years ago

Understood - in terms of transforming the addon further to be a standalone module, I definitely support, but let's not mix that into this pull request.

And, if you are trying to run the tests outside of the pure git repo here, then yes I'd expect it to not work, it's meant to work just for the repo itself (e.g. if you were to do a git clone of this repo).

neomonkeus commented 3 years ago

Yeah I am already monkeying about on a branch, this PR is way too bloated as is to even consider any of that. Just going to see how far I get as I am mainly interested to see if I can get the tests running to at least get that coverage/confidence.

TheDuckCow commented 3 years ago

Hey just an FYI, my goal is to merge this (and another parallel unrelated PR) this weekend! Letting you know in case you had anything local, but if not I can probably take it from here.

TheDuckCow commented 3 years ago

FYI I did one push to include your name as one of the author to give you credit (but to be clear, not expecting you to be a co-owner or anything like that!) Let me know if you prefer a different name or to not be mentioned at all. I'll aim to do the merge tomorrow.

neomonkeus commented 3 years ago

FYI I did one push to include your name as one of the author to give you credit (but to be clear, not expecting you to be a co-owner or anything like that!) Let me know if you prefer a different name or to not be mentioned at all. I'll aim to do the merge tomorrow.

Thanks for the inclusion as an author, happy out to be done a contributor. Probably something that could be included if the addon is converted to a python module at some point.

The PR is marked as Changes required, not sure if that is on my end or yours

TheDuckCow commented 3 years ago

'Changes required' was just to remind me of a bad merge, I've fixed it now. Merging now, will make some more changes for formalizing into a release. Thanks again for the entirely of the contribution here!