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

Implement and apply openscad-format #50

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

Maxattax97
Copy link

This automates the task of code formatting for the whole repo. Simply execute ./format.py

@MichaelAtOz
Copy link
Member

MichaelAtOz commented Mar 13, 2019

I think we probably want to discuss the formats before doing this.
While I haven't done much thinking yet (except indent should be 2 chars), I noticed this:
Capture format
Whitespace in the path is not good.
I don't think fiddling with the order (or position) of include & use is a good idea.

@Maxattax97
Copy link
Author

Maxattax97 commented Mar 14, 2019

Looks like a bug due to the dashes, I'll fix that, bump the NPM version, and update the results here. I'll also add some options for custom formatting... it uses some hacks with Clang to format it.

On order and position: the only edge case I can think if that would be beneficial is when you have an include/use inside of a module, e.g. for testing... is that right? The other issue I'm seeing is keeping them after header comments (at the top of files), but that's a relatively easy fix.

I just made the formatter the other day and thought I'd poke you folks for interest since it's a bigger repo.

EDIT: Also, from the README...

I'd prefer to have all included code nicely indented, at least at the block level, and no extraneous whitespace. I'm used to *indent with four spaces as opposed to tabs* or other mixes of whitespace, but at least try to choose a style and stick to it.

But I don't really know who manages the repo anymore.

@MichaelAtOz
Copy link
Member

MCAD repo is 'managed' by the same main OpenSCAD community, it gets little love, as changing it opens up a can of worms.
Two spaces is my personal preference, I mentioned it to poke some debate.
Order is critical any include<> or use<> could depend on one-another, position relative to comments less so, I agree header comments should go first.
Position relative to other code is critical, see here, also include<> can be used to bring in arbitrary text not just whole modules/functions etc & either could be in another scope (e.g module as you say).
So I wouldn't be moving anything.

@Maxattax97
Copy link
Author

Maxattax97 commented Mar 14, 2019

Since you bring the difficulty with changing MCAD, I'll bring up a repo I've been building this formatter (and also a language server based on pygls) for. My hope is to keep the convention very strict, the code base very stable, and offer a wide array of well documented modules/functions including HTML docs. I'm working on creating the other tools first, but some sample code is in the util folder.

But back on topic, what you've explained makes sense. The main issue with hacking Clang is the include/use syntax must be converted before it's parsed, however I think I have an idea. Fixes coming soon. 👍

@Maxattax97
Copy link
Author

That should fix the includes, and the indentation is now to your preference. Whatever you folks decide on, it can be changed via the .openscad-format configuration file, which follows clang-format's format which you can find here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants