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

Python3 fixes #99

Merged
merged 43 commits into from Sep 22, 2020
Merged

Python3 fixes #99

merged 43 commits into from Sep 22, 2020

Conversation

TrystanLea
Copy link
Member

Reopened against new python3 master branch

- Set the log dir mode with mkdir.
- Use Type=exec, it's more resilient to some failures (i.e. absent binary).
- Use a better name
- Use the default dependencies which seemed to be duplicated here anyway. Remove nonexistent syslog.target
- Remove PIDFile. It's unnecessary and systemd doesn't use it except for units of Type=forking
- Use a slower RestartSec (the default of 100ms is probably inappropriate)
Calls to logging functions should use %s/%d/etc and pass parameters
instead of interpolating first. The logging module can avoid doing that
work if the log message would be filtered anyway.
- Avoid import *
- Use setdefault for dict initialisation.
- Remove/simplify various bits.
This class is only ever a ConfigObj wrapper, so remove the json code.
This adds config options to control the log file rotation.
This import mechanism is fragile and needs to be converted into a true
module. Revert this for now.
This means that emonhub shuts down cleanly when stopped by, e.g.,
systemd.
- Standard library imports should go first.
- Remove unused or duplicate imports.
- expr == False -> not expr
- expr == True -> expr (or expr is True, if the type isn't clear).
- not x in y => x not in y
- not x == "1" => x != "1"
- omit range default
- 'if not x or not y: pass else: z' => 'if x and y: z'
- str.__len__ => len
- x > 1 and x < 10 => 1 < x < 10
- not x > y => x <= y
- if x: if y: if z: _ => if x and y and x: _
- BUGFIX: "a" and "b" in f => all(i in f for i in ["a","b"])
- BUGFIX: .lower => .lower()
- defaultdict to avoid initialising every entry up front
- Use struct.calcsize instead of copying the values.
- if x == 0: y = False: else y = True => y = bool(x)
- list[len(list)-1] => list[-1]
- math.pow(256, 1) => 0x08 etc.
- x = bytearray(); x.append(y); x.append(z) => x = bytearray([y, z])
- value = x; return value => return x
- Create dicts at compile time.
- Use startswith as it's less error prone.
- Add some FIXMEs.
- Make dict at compile time.
- Use enumerate.
- Lift sum out of if blocks.
It's much easier to use and it didn't change from python2 to python3.

I can't easily test PacketGen, though, but it seems to be unused.
We were passing a string where an integer port number was expected. Add
a cast.
This code shouldn't really be relying on division, but change it to
return an integer anyway.
- Set the log dir mode with mkdir.
- Use Type=exec, it's more resilient to some failures (i.e. absent binary).
- Use a better name
- Use the default dependencies which seemed to be duplicated here anyway. Remove nonexistent syslog.target
- Remove PIDFile. It's unnecessary and systemd doesn't use it except for units of Type=forking
- Use a slower RestartSec (the default of 100ms is probably inappropriate)
Calls to logging functions should use %s/%d/etc and pass parameters
instead of interpolating first. The logging module can avoid doing that
work if the log message would be filtered anyway.
- Avoid import *
- Use setdefault for dict initialisation.
- Remove/simplify various bits.
This class is only ever a ConfigObj wrapper, so remove the json code.
This adds config options to control the log file rotation.
This import mechanism is fragile and needs to be converted into a true
module. Revert this for now.
@TrystanLea
Copy link
Member Author

@bwduncan should I merge this?

@bwduncan
Copy link
Contributor

Yes, please do! I've been running it happily for some months. If the existing systems can be upgraded painlessly, I see no reason not to continue.

In case it's not clear, this code won't run with python 2.

@glynhudson
Copy link
Member

Can this be closed? Python 3 is now supported

@borpin
Copy link
Contributor

borpin commented Sep 21, 2020

@TrystanLea can we check and see if all this has actually been merged already and if so close this please.

@TrystanLea
Copy link
Member Author

This has not been merged yet, they key is that its python3 only, I think we are probably safe to merge (and resolve conflicts now).. but I cant be sure?

@borpin
Copy link
Contributor

borpin commented Sep 21, 2020

I think it might need to be rebased as some of it must have gone into master.

@TrystanLea
Copy link
Member Author

TrystanLea commented Sep 21, 2020

I've fixed the conflicts and pushed to a new branch, with pull request #132

Testing briefly here looks like its all working fine. Are we safe to merge?

Both the latest image and the Oct19 image upgrade fine to python3 so these should work fine with these changes.

Older systems are running the emon-pi branch which has not been updated, now that we have master and stable branches. Merging this pull request will not affect these systems and updates are disabled anyway for these older systems.

I think we are safe to merge this, can anyone see any issues with that?

@TrystanLea TrystanLea merged commit 5ee8439 into openenergymonitor:master Sep 22, 2020
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.

None yet

5 participants