Skip to content

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
@jimklimov jimklimov modified the milestones: 2.8.4, 2.8.5 Jul 29, 2025
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