Skip to content

add get_time and set_time in util#308

Merged
gijzelaerr merged 3 commits into
gijzelaerr:masterfrom
jialiang-yin:time
Sep 8, 2021
Merged

add get_time and set_time in util#308
gijzelaerr merged 3 commits into
gijzelaerr:masterfrom
jialiang-yin:time

Conversation

@jialiang-yin

Copy link
Copy Markdown
Contributor

Hello,
I have added the new functions in util. Could someone take a look at it?

@gijzelaerr

Copy link
Copy Markdown
Owner

first impression is that it looks good, thanks. there is only an issue with code style, as you can see in the github actions build:

snap7/util.py:617:18: E261 at least two spaces before inline comment
snap7/util.py:627:12: E127 continuation line over-indented for visual indent
snap7/util.py:657:109: W504 line break after binary operator
snap7/util.py:667:1: E302 expected 2 blank lines, found 1

@jialiang-yin

Copy link
Copy Markdown
Contributor Author

Hello @gijzelaerr,
I have modified the code style and pushed it again, github is checking the code. But one check (build-and-test-wheels / testing wheels (windows-latest, 3.7) (pull_request) ) is still pending after 30 mins. Could you please have a look at it?
Thank you.

@swamper123 swamper123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some smaller style things in my opinion.
A question I would have is, what happens if sb. sets a bigger/false time_string greater than 4 Bytes. Would it break succesfuly?

Comment thread snap7/util.py
>>> data
bytearray(b'\x8d\xda\xaf\x00')
"""
import re

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re is already imported at line 88. This line is not needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some smaller style things in my opinion.
A question I would have is, what happens if sb. sets a bigger/false time_string greater than 4 Bytes. Would it break succesfuly?

Good catch. I didn't notice that issue. I will modify this and make a new pull request.

Comment thread snap7/util.py
minutes = seconds // 60
hours = minutes // 60
days = hours // 24
time_str = str(days * sign) + ":" + str(hours % 24) + ":" + str(minutes % 60) + ":" + str(seconds % 60) + "." + str(milli_seconds)

@swamper123 swamper123 Sep 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string concatination style still works, but is a bit outdated (but not wrong). An f-string equivalent would be sth. like this:

time_str = f"{days * sign!s}):{hours % 24!s}:{minutes % 60}:{seconds % 60!s}.{milli_seconds!s}")

@gijzelaerr

Copy link
Copy Markdown
Owner

regarding the pending CI build, you can ignore that, it just hangs sometimes.

@gijzelaerr gijzelaerr merged commit f637f0e into gijzelaerr:master Sep 8, 2021
@gijzelaerr gijzelaerr added this to the 1.2 milestone Sep 8, 2021
Comment thread snap7/util.py
Comment on lines +659 to +661
formatstring = '{:0%ib}' % bits
byte_hex = hex(int(formatstring.format(time_int), 2)).split('x')[1]
bytes_array = bytes.fromhex(byte_hex)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these conversions necessary? Int has a method .to_bytes() that does the same. Or it might be better to use a struct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You approach is more elegant. Thank you.

Comment thread snap7/util.py

Examples:
>>> data = bytearray(4)
>>> snap7.util.set_dint(data, 0, '-22:3:57:28.192')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo. Wrong method.

@jialiang-yin

Copy link
Copy Markdown
Contributor Author

This version has one more problem. If the time_string is '-0:0:0:0.1', the result bytearray is not 0xffffffff, but 0x00000001.

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.

4 participants