Skip to content

Style guide says 4 spaces, yet you use 2. Not Documented #1396

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

Closed
mswezey23 opened this issue Oct 8, 2018 · 4 comments
Closed

Style guide says 4 spaces, yet you use 2. Not Documented #1396

mswezey23 opened this issue Oct 8, 2018 · 4 comments
Labels
bug documentation Inline comments, guides, and examples.

Comments

@mswezey23
Copy link
Contributor

mswezey23 commented Oct 8, 2018

Style guide says 4 spaces, yet you use 2.
https://github.com/OpenZeppelin/openzeppelin-solidity/blob/f7e53d90fa638553ffc93e93fe1b12fc081bb774/.soliumrc.json#L7

Not Documented here as to why:
https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/CODE_STYLE.md
If fact you state your follow the style guide.

💻 Environment

VS Code

📝 Details

Very painful and time consuming to refactor code to meet your standards (for PR) when the style guide suggests 4 spaces vs only 2. The standard among the majority of any language is typically 4.

🔢 Code to reproduce bug

Solidity Linter in VS Code - yarn lint

@nventuro nventuro added bug documentation Inline comments, guides, and examples. labels Oct 8, 2018
@nventuro
Copy link
Contributor

nventuro commented Oct 8, 2018

Hey there @mswezey23!

This has already been brought up a couple times (e.g. #152), I quote my thoughts from that issue:

Maybe we should change the recommended style? :) After all, Solidity is supposed to be influenced by JavaScript (and there is a lot of JS around anyway due to tests, deployment scripts, etc.), where the 2 spaces convention is very widely extended.

You are right in that this is not documented in the style guide, as it should (@ElOpio could you take care of this?). We do haver our linter set up to enforce this, though.

I'm surprised however that you find it time consuming to refactor code with 4 spaces, I'd expect most editors to be able to handle this automatically.

@mswezey23
Copy link
Contributor Author

mswezey23 commented Oct 8, 2018

I'll have to check my setup, but attempting to have it auto refactor was not having the desired impact.

Question: What is the majority of the Solidity community utilizing? 4 or 2 spaces? Does it makes sense to go against the standard or the majority in this instance?

Side note: I'm not HUGE in the JS community (coming more so from a C#/Java background) . Given that, 2 spaces being the standard is news to me 🤣

If the push is for 2 spaces, which it sounds like from thread you posted earlier, is there any active movement or effort to get this adopted by the style guide?

@nventuro
Copy link
Contributor

After some discussion with @frangio, we've decided to migrate to 4 spaces, since interoperability with tools/other projects is more important than our personal preferences. Thanks @mswezey23, @LogvinovLeon, @treeder and @kabl for pushing for this!

@mswezey23
Copy link
Contributor Author

Ah! :) This makes me so happy

Thanks for the update @nventuro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug documentation Inline comments, guides, and examples.
Projects
None yet
Development

No branches or pull requests

2 participants