Skip to content

Conversation

@versat
Copy link
Collaborator

@versat versat commented Oct 22, 2019

No description provided.

@versat
Copy link
Collaborator Author

versat commented Oct 22, 2019

I have thoroughly tested the server locally, but I appreciate any reviews, tests and so on.

@versat
Copy link
Collaborator Author

versat commented Oct 22, 2019

Todo: Ignore donate-cpu-server.py for pylint (Python 2) analysis. It will fail since it is Python 3 only.
Add pylint (Python 3) analysis for all packages.
I am working on this.

EDIT: This is done now

Copy link
Contributor

@rikardfalkeborn rikardfalkeborn left a comment

Choose a reason for hiding this comment

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

Need to update tools/test/start_donate_cpu_server_test_local.sh to use python3 instead of python?

@versat versat force-pushed the donate-cpu-server_python3 branch from c694964 to 34240d3 Compare October 23, 2019 06:34
@versat versat requested a review from danmar October 23, 2019 10:54
@versat versat force-pushed the donate-cpu-server_python3 branch from 45d5f70 to 99c5826 Compare October 23, 2019 12:54
return datetime.datetime.now().strftime('%Y-%m-%d %H:%M')


def dateTimeFromStr(datestr):
Copy link
Owner

Choose a reason for hiding this comment

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

How about def dateTimeFromStr(datestr: str):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK this would require at least Python 3.5. I am not sure if everyone who develops on this script has such a recent version. Personally I like these typeinfo hints.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK this would require at least Python 3.5. I am not sure if everyone who develops on this script has such a recent version. Personally I like these typeinfo hints.

That is not quite right. The function annotations itself are available since Python 3.0, see https://www.python.org/dev/peps/pep-3107/.
Type hints (see https://www.python.org/dev/peps/pep-0484/) are available since Python 3.5.
So it looks like it is acceptable to add function annotations like you suggested (and I also like better).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added Python 3.0 function annotations instead of the comments with type hints now.
For the client script, which should be Python 2 compatible, we could use the comments with type hints. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

yes in the client script the comments are fine.

The following commands were used for these changes:
futurize -1 -w donate-cpu-server.py
2to3 -w donate-cpu-server.py
Manually fixed the Unicode issues. Received data is decoded, sent data
is encoded.
This enables better static analysis and suggestions in an IDE.
donate-cpu-server.py is only Python 3 compatible, so it must be ignored
for pylint verification under Python 2.
All Python scripts that were verified with pylint under Python 2 are
now also verified with pylint under Python 3.
Since the server script is executable now and has a shebang it can
be directly executed.
@versat versat force-pushed the donate-cpu-server_python3 branch from 99c5826 to 62205e7 Compare October 25, 2019 06:34
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

Thanks! Feel free to merge this

@versat versat merged commit ec521fb into danmar:master Oct 26, 2019
@versat versat deleted the donate-cpu-server_python3 branch October 26, 2019 19:10
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.

3 participants