search

ggtracker / sc2reader

Extracts gameplay information from Starcraft II replay files
http://pypi.python.org/pypi/sc2reader
MIT License
145 stars 144 forks source link

Enforce python code formatting #86

Closed StoicLoofah closed 5 years ago

StoicLoofah commented 5 years ago

See discussion #82 where @dneise proposed using https://github.com/python/black

I don't have strong feelings about whether we should globally reformat the codebase or if we should just apply it progressively as we're making changes, but we should document and verify code if we decide to implement this.

dneise commented 5 years ago

Hello Kevin, thanks for opening this discussion. I personally believe following any kind of code style will help a lot.

I personally would refrain from enforcing anything in an open source project. I'd rather invite people to follow a certain style. And if PRs come in from people who have never heard of code style or tools to help them with style, I'd show them. Since repo owners can push into PRs from forks it is nowadays really easy to train newbies: While in the past one needed to tell them to about style issues in their PR and weit for them to fix the issues before merging, one can nowadays just apply ones preferred styling tool one the PR .. push it and then show the original author of a PR, that in the future one would rather like them to apply this kind of style to the code prior to opening a PR.... that's it .. matter of minutes .. at most.

Now which style one should use, I do not know .. I also do not care. I must admit I was just to lazy to switch off my auto-blacken feature in my editor.

If you'd like to see how black would reformat the entire codebase: https://github.com/ggtracker/sc2reader/compare/upstream...dneise:blacken_everything

dneise commented 5 years ago

I have one question though:

we should document and verify code if we decide to implement this.

What do you mean by document and verify in this context?

cclauss commented 5 years ago

Document means write guidelines in the README/contributing doc.

Verify would be something like https://github.com/cclauss/claussoft-dominos/blob/master/.travis.yml#L15 which fails the build if the code is not 100% black.

dneise commented 5 years ago

Ah okay, got it. So .. I've made a proposal in PR #87, maybe somebody would like to have a look at it. I had some problems with the circleci config, but maybe that's better discussed over there.

StoicLoofah commented 5 years ago

Done in #87