Skip to content

ref: python3 conversion#2

Open
patthiel wants to merge 8 commits intokangsterizer:masterfrom
patthiel:ref/python3
Open

ref: python3 conversion#2
patthiel wants to merge 8 commits intokangsterizer:masterfrom
patthiel:ref/python3

Conversation

@patthiel
Copy link

@patthiel patthiel commented Oct 2, 2023

  • ran modernize on various python files to fix the compatibility issues
  • going through the toil of fixing various indentation issues
  • will eventually add some unit tests
  • Updated dockerfile to run python3.9, i'll eventually bring this up to 3.11
  • With any luck, github co-pilot will help me spot more issues

Copy link
Owner

@kangsterizer kangsterizer left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Just a couple of nits - but it would also be helpful if you were able to split the commits in 2:

  1. the tab/space conversions
  2. the rest of the p3 conversion

this allows the diff to be more readable and its easier to catch any possible issue

otherwise, ill take another quick look when possible

@patthiel
Copy link
Author

patthiel commented Oct 2, 2023

thanks for the review, I totally didn't mean to PR this against upstream yet. I meant to PR it against my own fork as a way to track my progress. Good feedback though, i'll make sure to separate out the tab/space stuff vs. the rest

@patthiel
Copy link
Author

patthiel commented Oct 3, 2023

Made some progress..

Now it's a matter of trying to connect and seeing what breaks.

To test i've been running it in th container with:

docker run --rm -v $(pwd):/app -p 5500:5500 -p 6667:5500 phxd:latest

With the latest changes, it'll start the server, but i haven't successfully managed to connect yet. Still working through the issues.

To see the diff without the whitespace changes: 509df1b?diff=unified&w=1

# this is an avaraline extension for nick coloring
if self.color >= 0:
data += pack( "!L" , self.color )
# if self.color >= 0:
Copy link
Author

Choose a reason for hiding this comment

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

need to fix

@kangsterizer
Copy link
Owner

come back! ;)

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.

2 participants