search

openairplay / airplay2-receiver

AirPlay 2 Receiver - Python implementation
2.13k stars 133 forks source link

Fix some generic style-related issues #33

Closed TheSpookyCat closed 3 years ago

TheSpookyCat commented 3 years ago

Almost everything changed in this PR is style-related or "best practice"-related.

It does not change functionality in any way, nor does it fix every issue of this type that I could find. It does however resolve most, and results in my scrollbar looking much tidier.

systemcrash commented 3 years ago

I think we both had the same idea 🤣 Can you rebase?

glmnet commented 3 years ago

are we using a known formatter? which?

systemcrash commented 3 years ago

As long as it's pep8 (some exceptions like line length, among others) then it's good.

TheSpookyCat commented 3 years ago

Should be good to go

systemcrash commented 3 years ago

Two problems: 1 if I merge this, all commits will be squashed, which we don't want with the code changes for the int -> str in http you did. We want separate commits, because this is a behaviour change, which might need reverting later in the unlikely event it causes problems. But one should operate from this principle with commits. See 2. 2 (I think) you merged master into your HEAD, rather than re-based (your branch onto where master is here), which means commit history here will be overwritten for those who base work on the latest commit here if we merge this (manually via git, and not here in github, since the two behaviours differ slightly at merge, and I cannot merge without squash in github as it is now)

If I manually stitch these commits in, and e.g. cherry pick them, then the history goes into my name :/

Tell you what: if you separate the behaviour changes into a separate PR (and/or optionally re-base), we can merge as two separate commits.

Otherwise, the content is good and the changes are welcome. We want to maintain your contributions also 😄

TheSpookyCat commented 3 years ago

I think I rebased using my IDE, something I've not done before, and instead of rebasing it merged? Weird but understood - I'll close this and open up some others separately.