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

PEP8 cleanup. #512

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

PEP8 cleanup. #512

wants to merge 3 commits into from

Conversation

Cabalist
Copy link

@Cabalist Cabalist commented Jan 3, 2018

PEP8. Updated code to be more compliant with Python3. Renamed a couple things that were colliding with reserved kws. Use "in" instead of ".has_key()"

The app has dependencies on an older GTK library that will take some time to replace. I am submitting this pull request as laying the groundwork for that cleanup. Thanks!

…e things that were colliding with reserved kws. Use "in" instead of ".has_key()"
@clepple
Copy link
Member

clepple commented Jan 15, 2018

Sorry I haven't had a chance to look at this until now. I just checked out this branch on Debian stretch, and did a quick regression test using Python 2.7. Relative to the NUT-Monitor script in stretch (from NUT 2.7.4), your changes do not seem to allow saving favorites. Let me know if you need an exact traceback, but I think it had a problem trying to save the False boolean value to "auth" - something about values only being strings.

@jimklimov
Copy link
Member

A few merge conflicts cropped up :(

@jimklimov jimklimov added need testing Code looks reasonable, but the feature would better be tested against hardware or OSes and removed merge-conflicts labels Oct 25, 2020
@jimklimov
Copy link
Member

jimklimov commented Oct 25, 2020

I fixed the merge conflicts to make this PR applicable to master, to the best of my understanding of the script (in particular, there was a bit about renaming the variables to avoid reserved key words).

Still, marked as "Needs testing" to have someone check the GUI tool changes before merging.

CC @melenki as a recent contributor to NUT-Monitor area: Alexey, would you please have a chance to revise this PR and test its version of the scripts on a GUI workstation to make sure stuff still works? ;)

@lgtm-com
Copy link

lgtm-com bot commented Oct 25, 2020

This pull request fixes 2 alerts when merging f8374c0 into 52df79c - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@melenki
Copy link

melenki commented Oct 26, 2020

[...]

CC @melenki as a recent contributor to NUT-Monitor area: Alexey, would you please have a chance to revise this PR and test its version of the scripts on a GUI workstation to make sure stuff still works? ;)

Since the patch was published, the Python version has changed, and so has the ABI.
NUT-Monitor does not start after applying the patch. A number of language constructs for Python versions 3.6/3.8 require refactoring. For example, the method 'glade' is not present in the Gtk module for Python3.

To fully meet the requirements of Python 3.x, a patch needs to be reworking.

@jimklimov jimklimov added the needs-work PR discussion concluded that some work is needed from the contributor label Sep 21, 2021
@lgtm-com
Copy link

lgtm-com bot commented Sep 21, 2021

This pull request fixes 2 alerts when merging 73efe9c into 8e86544 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@jimklimov jimklimov added this to the 2.8.1 milestone Apr 27, 2022
@jimklimov
Copy link
Member

Note regarding comment for Python 3: it is a different language than Python 2 :)

That said, several layers of PRs were made to make some code dually supported on both, and reimplement the GUI for the new tech, e.g. #1310

@jimklimov jimklimov modified the milestones: 2.8.1, 2.8.3 Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflicts need testing Code looks reasonable, but the feature would better be tested against hardware or OSes needs-work PR discussion concluded that some work is needed from the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants