search

MovingBlocks / TeraMisc

Support repo for Terasology - for stuff we don't want to keep in the main repo
https://github.com/MovingBlocks/Terasology
6 stars 21 forks source link

pep8 format #58

Closed monuelo closed 3 years ago

monuelo commented 5 years ago

Apply pep-8 format on blender add-ons python files

monuelo commented 5 years ago

@Cervator

Cervator commented 5 years ago

Hey @hericlesme - could you explain what that actually means? I've never heard of pep-8 before. It is good to preface your PRs with some description on what why how etc :-)

It might be worth coordinating on this via chat first. The https://github.com/MovingBlocks/BlenderAddon repo is actually meant to be the path forward on the Blender addons. Reformatting everything here would likely make comparisons very difficult. Would be nice to know that the new repo is fully functional then we can delete this instead and reformat the new repo if needed.

monuelo commented 5 years ago

@Cervator PEP 8 is a style guide for python code. A set of code conventions. I just applied the formatting according to the layout rules. You can see more about this here.

monuelo commented 5 years ago

If you want, I can apply the formatting in the BlenderAddon repository as well.

Cervator commented 5 years ago

Yeah I was able to find that out by Googling it. But not knowing Python I have no idea how many types of formatting there are for Python, or if any of them are "right" - that risks getting into a "tabs!" vs "spaces!" type match. Imagine if somebody popped up one day and reformatted an entire codebase to one or the other without any posted reasoning ... 😁

In other words: When making a PR like this I suggest asking on chat first or adding some background to your PR's description in case anybody likely to review may not have some assumed knowledge :-)

The risk here is: very few people know this code (fewer yet the unified version - likely exactly one person, who is low on time). Without being sure we have somebody new able to work on it a mass reformat may throw off the few contributors that know where to find something specific, or remember how a particular change affected things. Diffs from after this PR vs older history would vary wildly.

Not saying this isn't the correct action, but trying to provide some "Oh yeah!" realizations of what else may need to be kept in mind :-)

The follow-up question then: Are you interested in working on the actual addon, learning how it does its thing, and improving it so others will be able to more easily work on the (reformatted) code?

monuelo commented 5 years ago

Are you saying to join the group of very few people that know this code? I do not know if I have the necessary experience of how it works yet. I just saw the code in python and checked if it followed the PEP 8 standards. But what you said makes perfect sense. In this case, what do you have to tell me/recommend?

Cervator commented 5 years ago

I would love for you to join that group! We badly badly need more work on this addon :-)

But in the case where you're not able to join, jumping in from out of nowhere to throw the code in a formatting blender then running away again cackling may not be ideal, hehe 😁

On the plus side we can leave this PR here for a bit to see if anybody else will chime in, we've got plenty of contributors who would have more strong opinions about Python stuff than me 👍

monuelo commented 5 years ago

I can study the addon and leave the PR here. Any further tips on how I can go into it?

Cervator commented 5 years ago

Yep it is documented over at https://github.com/Terasology/TutorialAssetSystem/wiki#blender-addon-for-creatures - or at least the creature one is. I'm not sure how much the unified addon has changed, might want to try catching Michael Pollend on chat to ask.