TaylorSMarks
Cleaning up the codebase
When contributing I found navigating some of the code difficult because of the inconsistencies in its formatting and other styling issues.
As the first point I think all support for python2 should be dropped and modern (python 3.6) features should be made use of, for users that still need compat with previous versions the older versions are still available.
I'm not familiar with the complete structure of the current playsound module but I think it'd be clearer to have a module per OS implementation of the function with its own imports that is conditionally imported in the main module instead of the current approach with everything in one place leading to a bit messy structure because of the nesting necessary.
The Path handling currently immediately converts the accepted path to a string and then continues to do manual or os.path operations on that string, which could be made clearer by doing this the other way around and using the Path object's methods and only converting it to a string when it needs to be passed to an external function
Then there are general issues like wrong docstring quotation marks etc. and PEP 8 violations such as aligning statements with spaces. The naming seems to be mostly consistent on camelCase so that could be stuck to with the current snake_case renamed to that.
There are also some lines of code like https://github.com/TaylorSMarks/playsound/blob/9cf4af20caa5ae8586f88b65659681b24f0c4e69/playsound.py#L208 which seem questionable in their implementation and could be made clearer
If you think this is worth doing and agree with the proposals, I'd be willing to make the changes
Sorry, I’m not interested.
1 - I want to maintain Python 2 compatibility until ~7 years after the major OSs stop including it.
2 - Reformatting muddies git history.
3 - I disagree with a lot of pep8.
If you’re itching to make changes to playsound, you could add support for KDE (I left details for what I think it entails in another issue that’s still open) or you could improve how the tests run on Travis-CI so that they don’t require mocking anymore.
I’m split on splitting it into more files. If I knew it’d grow to what it is, I would have split it up earlier. But at this point, it’d muddy the git history.
Adding support for KDE could be done in a new file.
Sorry if I come across as a jerk. I’m happy you’re interested in helping and about the contributions you’ve made already.
On Aug 15, 2021, at 20:03, Numerlor @.***> wrote:
When contributing I found navigating some of the code difficult because of the inconsistencies in its formatting and other styling issues.
As the first point I think all support for python2 should be dropped and modern (python 3.6) features should be made use of, for users that still need compat with previous versions the older versions are still available.
I'm not familiar with the complete structure of the current playsound module but I think it'd be clearer to have a module per OS implementation of the function with its own imports that is conditionally imported in the main module instead of the current approach with everything in one place leading to a bit messy structure because of the nesting necessary.
The Path handling currently immediately converts the accepted path to a string and then continues to do manual or os.path operations on that string, which could be made clearer by doing this the other way around and using the Path object's methods and only converting it to a string when it needs to be passed to an external function
Then there are general issues like wrong docstring quotation marks etc. and PEP 8 violations such as aligning statements with spaces. The naming seems to be mostly consistent on camelCase so that could be stuck to with the current snake_case renamed to that. There are also some lines of code like https://github.com/TaylorSMarks/playsound/blob/9cf4af20caa5ae8586f88b65659681b24f0c4e69/playsound.py#L208 which seem questionable in their implementation and could be made clearer
If you think this is worth doing and agree with the proposals, I'd be willing to make the changes
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.
I want to maintain Python 2 compatibility until ~7 years after the major OSs stop including it.
I guess I understand, but what's wrong with dropping it only for new vers? playsound worked fine for them so far and the current versions will continue to do so, from the jetbrains/PSF developer survey it's evident that the usage of python 2 is dropping off significantly so those who are still using it with playsound either have it working properly or fixed their use cases locally
Reformatting muddies git history. I disagree with a lot of pep8.
What part of the history are you concerned about? For the git blame, the commits can be included in a .get-blame-ignore-revs file; as for going through it as a list, most the current commits seem to focus on fixing small mistakes which don't exactly offer much more than a reformatting commit.
I'm not saying the project should conform to PEP8, but some consistency in the style it uses would be nice.
I’m split on splitting it into more files. If I knew it’d grow to what it is, I would have split it up earlier. But at this point, it’d muddy the git history.
In my opinion the tradeoff of losing the linear history on the transition would be worth the clarity splitting the module up into a package brings.
What do you think of the functional changes to use more idiomatic code and to make some interfaces like winCommand more consistent?