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

Consistent style of code #25

Closed
pktiuk opened this issue Sep 6, 2020 · 5 comments · Fixed by #47
Closed

Consistent style of code #25

pktiuk opened this issue Sep 6, 2020 · 5 comments · Fixed by #47
Assignees

Comments

@pktiuk
Copy link
Member

pktiuk commented Sep 6, 2020

Currently, there is some mess in style of code (as a good examples can be usage of if (... and if(... )

TODO:
Create configs for code formatter which will help with ensuring quality and consistency of code style.
Format code (entire or at least some pieces)

@pktiuk
Copy link
Member Author

pktiuk commented Sep 26, 2020

  1. I think the best tool to ensure proper formatting would be clang-format, because it is quite popular, is easy to configure and customize. It is also easy to experiment with it using sites like https://zed0.co.uk/clang-format-configurator/

  2. Now we should decide whether do we want to use one of the simplest configuration (one of available presets with only some minor tweaks), or try to stick to current style of code as close as possible to minimize the number of changed lines.

  3. After selecting configuration we can further deal with this task on several ways:

  • format everything
  • format only files we edited
  • format only these parts of files we have changed
    Every of these options has disadvantages and advantages.

@gombosg
Copy link
Collaborator

gombosg commented Sep 26, 2020

Nice!

I personally don't care at all about style settings, but love consistent, enforced and automatically-applied style. (Can VSCode pick up these format settings?)

I think if you configure to have braces { on a newline then it would greatly decrease changed lines. Also, if you allow a bit more characters than the default (80?), like 110, then that would also decrease changed lines.

I think a total reformat is fine, otherwise we'd have to distinguish reformats from new code inside upcoming PRs which is more burden.

Nicer code may help other contributors in the future as well to get started. (Along with dead code and file removal)

If everything is reformatted in 1 commit, you could add a style enforcing step to the compilation, thus enforcing consistent style in the future which IMO is a nice bonus.

@pktiuk
Copy link
Member Author

pktiuk commented Sep 26, 2020

Yes, config file can be used with IDE in my case (VsCode I use add-on which loads config from this file automatically, we could also use in VScode option Format on save to ensure proper formatting in edited files/lines of code).

I will try to make this config better suited for current style, and then we will check how big part of code will be reformatted.
I would like to avoid reformatting everything at once, because editing too many lines of code may hide original commits for some lines of code (I use git-lens to read these comments, and they make understanding some pieces of code much easier).
image

@gombosg
Copy link
Collaborator

gombosg commented Sep 27, 2020

Yeah I use GitLens a lot, too. Blame is useful. 🙂 Definitely, in this case, the reformat commit could take the last commit's place, but you have gitlens.blame.ignoreWhitespace exactly for that (try committing a few spaces in a line, it won't appear as last commit).

So if you manage to adapt the style so that it mostly just fixes whitespaces (indentation, whitespace around operators etc.) then even a full reformat could be a viable option.

@pktiuk
Copy link
Member Author

pktiuk commented Sep 27, 2020

Definitely, in this case, the reformat commit could take the last commit's place, but you have gitlens.blame.ignoreWhitespace exactly for that

Wow! I haven't been aware of this option. It's great :D

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 a pull request may close this issue.

2 participants