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

modernize #568

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

modernize #568

wants to merge 2 commits into from

Conversation

prusnak
Copy link
Contributor

@prusnak prusnak commented Oct 7, 2024

similar to markqvist/LXMF#20

@faragher
Copy link
Contributor

faragher commented Oct 7, 2024

So, my eyes glazed over about halfway through, so I'll have to go back later, but first blush:

Are IOError and OSError identical in all architectures and on all platforms?

Same question with the equivalence of b'foo' and foo.encode("ascii")?

@prusnak
Copy link
Contributor Author

prusnak commented Oct 7, 2024

Are IOError and OSError identical in all architectures and on all platforms?

Yes. It is just an alias. Quoting from the documentation:

Changed in version 3.3: EnvironmentError, IOError, WindowsError, socket.error, select.error and mmap.error have been merged into OSError, and the constructor may return a subclass.

Try this:

>>> IOError
<class 'OSError'>

Same question with the equivalence of b'foo' and foo.encode("ascii")?

Yes

@markqvist
Copy link
Owner

I really appreciate your willingness to help, but I'm just going to kindly ask if you read the Contribution Guidelines?

Also this one for reference.

@prusnak
Copy link
Contributor Author

prusnak commented Oct 7, 2024

@markqvist I understand the part about down-prioritizing formatting PRs in favor of PRs that provide functional benefits, but since there are no open PRs, I thought it might be worth a shot. If you do not like the changes for whatever reason, you can close the PR and I will carry on. No problem. You are the maintainer after all. :-)

@markqvist
Copy link
Owner

Looking at your PR, it's seems like you're putting the proper attention into doing everything well and consistently, and the changes are definitely for the better, so in this case, it's mostly that it will probably take a while before I get around to reviewing it all. Because I do have to go over everything meticulously to ensure that nothing bad snuck in, which in this case is going to take a bit of effort ;)

Another thing I'd like you to be aware of is that this also needs to compile for Android via P4A, which has some weird issues with functions inside f-strings in certain cases. So please make sure that it actually works on that platform too. You should be able to confirm it simply by compiling the Sideband APK with your changes in it.

@markqvist
Copy link
Owner

Oh, and forgot, the "xyz".encode("ascii") are a no-go. Please keep them as b"xyz".

@prusnak
Copy link
Contributor Author

prusnak commented Oct 8, 2024

Oh, and forgot, the "xyz".encode("ascii") are a no-go. Please keep them as b"xyz".

I did the conversions the other way around from encode() to b"".

@markqvist
Copy link
Owner

Hah! Okay! There you go, didn't even think there were any left. My mistake!

@Erethon
Copy link
Contributor

Erethon commented Oct 8, 2024

Is this maybe a chance to settle on a format style and enforce it by checking for non-compliance from now on?

@prusnak
Copy link
Contributor Author

prusnak commented Oct 8, 2024

Is this maybe a chance to settle on a format style and enforce it by checking for non-compliance from now on?

If this is what @markqvist wants, I can create another PR which reformats using black and adds a CI check that checks the format. In either case this should be in another PR, not this one.

@prusnak prusnak force-pushed the modernize branch 3 times, most recently from 011448c to 86d4028 Compare October 9, 2024 09:13
@prusnak
Copy link
Contributor Author

prusnak commented Oct 9, 2024

I went through my changes and now I am satisfied with the PR. Removing draft status.

@prusnak prusnak marked this pull request as ready for review October 9, 2024 09:20
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