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

Add a line length guideline to the GDScript style guide #2994

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Dec 20, 2019

From consensus in the issue and Discord, a soft limit of 80 characters per line seems to be the way to go.

In the future, we should also document multiline wrapping using parentheses and backslashes and make a recommendation. My vote goes for recommending parentheses whenever possible, as they're less error-prone compared to backslashes 🙂

This closes #2567.

@Calinou Calinou force-pushed the add-gdscript-line-length-guideline branch from ae1e084 to bc47ace Compare December 20, 2019 18:07
@Calinou Calinou changed the title Add a line lenght guideline to the GDScript style guide Add a line length guideline to the GDScript style guide Dec 20, 2019
@akien-mga
Copy link
Member

akien-mga commented Dec 21, 2019

I tend to find line length limitation in Python quite limiting and forcing to do a lot of wrapping all the time, which IMO doesn't improve readability much over simply allowing lines of 100-120 chars.

I like a line length limit of around 80 chars for blocks of English text, e.g. for our documentation rst, as it makes reading much easier. But for code, I don't think it has much of an impact on readability as long as the line length does not imply having to scroll horizontally. With the kind of screen real estate used by most editors, we can definitely go beyond 80 chars without any issue.

But if the majority wants a hard limit, I don't mind. But then the page should also explain how to wrap lines which would otherwise exceed the line length limitation. Should things be aligned with previous lines, if so following what logic? Or should there be an extra level of indent, or two? How to break lines is also not always straightforward in GDScript (when can you do it simply and when do you need \?), so this needs to be covered by the style guide.

@NathanLovato
Copy link
Contributor

NathanLovato commented Dec 21, 2019

Same as Akien here, 80 characters is too short. I haven't seen too many python projects actually follow this guideline.

80 characters prevent you from having even just 2 conditions on a given line in input functions or just doesn't work with some statements with the longer parameters/class names and type hints in Godot.

Here are three examples from our current project:

onready var _playback: AnimationNodeStateMachinePlayback = animation_tree["parameters/playback"] # 97 characters
	animation_tree["parameters/move_ground/blend_position"] = direction.length() # 81 characters
	var forwards: Vector3 = player.camera.global_transform.basis.z * input_direction.z # 83 characters

I have plenty of them. Type hints make for long lines.

I also agree that we'd need more guidelines for line-wrapping. There, I find black, the Python Software Foundation's formatter, great: how black wraps lines. It uses simple and consistent rules, like it wraps parentheses or brackets the same way, making it easy to read the code.

Another thing is that right now we'd have to enforce and review it manually, which is a time sink. The gdscript formatter wasn't ready for that last time I tried it. It'd save a lot of time if we could automate that step.

@Scony
Copy link
Contributor

Scony commented Dec 21, 2019

@NathanLovato I'm working on it: https://github.com/Scony/godot-gdscript-toolkit ATM I do linter, but soon I'll start with formatter (similar to black and clang-format)

@NathanLovato
Copy link
Contributor

@Scony Nice! Did you see there's a PR open for a formatter in the engine already? It's exposed to the command-line and is meant to follow the gdscript guidelines. How about joining forces there? godotengine/godot#27382

@Scony
Copy link
Contributor

Scony commented Dec 21, 2019

@NathanLovato I've got some strong reasons to not implement it in the engine, but haven't seen this PR anyway. I'll try to keep feature-parity with the one from PR ;-) And ofc. participate in discussions. Thanks.

@NathanLovato
Copy link
Contributor

NathanLovato commented Dec 21, 2019

@Scony is there something you'd take contributions for? We use python with the team and although I don't have experience with parsers I'd gladly help.
Sorry for the HS thing, but if there are things you'd need help with, I'll look at the issues you'll open in your tracker!.

@Scony
Copy link
Contributor

Scony commented Dec 21, 2019

@NathanLovato sure! there are plenty of things to do like linter checks (ones commented out in https://github.com/Scony/godot-gdscript-toolkit/blob/master/gdtoolkit/linter/__init__.py), formatter, metrics calculator (like python's radon - https://radon.readthedocs.io/en/latest/) + more linter checks that need to be invented. Whatever you want to do feel free to do - just add an issue and mention you're working on it :)

@Calinou
Copy link
Member Author

Calinou commented Jan 2, 2020

Based on your feedback, should I move the hard limit to 100 characters and remove the soft limit? We can then change the default line length guideline in the Godot editor and enable it by default.

@NathanLovato
Copy link
Contributor

In another discussion, people rightly mentioned that if we have guidelines for optional typing, they should stay separate. In the case of line length, you need to increase the limit with the extra type hints. So no need to take that into account.

I think it's good, I'll merge and tweak the phrasing a tiny bit. Thanks!

@NathanLovato NathanLovato merged commit 373f662 into godotengine:master Jan 2, 2020
@NathanLovato
Copy link
Contributor

The rationale being that we can change that bit anytime, people can open a discussion or a PR if they think it's unreasonable.

@Calinou Calinou deleted the add-gdscript-line-length-guideline branch May 4, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a line length guideline to the GDScript style guide
4 participants