8000 Style guidelines issue · Issue #111 · SGpp/SGpp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Style guidelines issue #111

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.

Sign up for GitHub
8000

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

Jump to bottom
Open
spc90 opened this issue Apr 8, 2019 · 3 comments
Open

Style guidelines issue #111

spc90 opened this issue Apr 8, 2019 · 3 comments
Assignees
@spc90 @leiterrl
Labels
discussion General problem for which possible solutions need to be discussed feature request Desirable, nice-to-have feature

Comments

@spc90
Copy link
Member
spc90 commented Apr 8, 2019

The back-and-forth discussion in one of the recent pull requests (#105, already merged) has raised an issue with respect to IF and HOW we enforce the usage of the style guidelines we mention on our website.

There are numerous files on the master branch that do not comply to the style guidelines and any minor change to those files has the potential to transform itself into a huge git diff after proper formatting, making reviews of pull requests a nightmare. As far as I know, we run only our flavor of cpplint as a prerequisite for accepting pull requests and we do not run clang-format. As a result we have lines like these, where the spacing and/or indentation is all wrong:

  /**
  * Generates a regular sparse grid of level levels, without boundaries
  * where dimensions are splitted into a groups with only certain number
  * of dimensions completely connected in a clique
  *

So then I propose this topic of discussion:

Do we want to properly follow the style guideline?

If yes, this would first of all require some immediate steps to tidy-up what we currently have, i.e. a pull request to make the current master fully compliant with the style guidelines (by running a formatter on all files).

Secondly, Jenkins should run a formatter by default (not just cpplint) and new pull requests should only be validated if the proper formatting is used. With a master branch that complies with the style guidelines, this prerequisite would guarantee a nice and tidy git diff. This could be enforced by a git pre-push hook, for instance.

If we want to use a style guideline but we're not willing to enforce it, then we would always get into issues of huge git diffs, or having to make periodical style fix pull requests, neither of which is something desirable.
@spc90 spc90 added discussion General problem for which possible solutions need to be discussed feature request Desirable, nice-to-have feature labels Apr 8, 2019
@leiterrl
Copy link
Member

I agree that enforcing the use of clang-format is probably the right way to go. This does add a little more friction for new developers but I think it is less cumbersome for everybody than the alternative. However, this implies that we find a linter for Jenkins that completely agrees with clang-format. Currently, this doesn't seem to be the case for cpplint...

@spc90
Copy link
< 97A9 /div>
Member Author
spc90 commented Apr 12, 2019

For new developers this should not be a problem. We have to mention the style we use in the wiki instructions and how they can run it locally, and if this is an automated check in github/jenkins for pull requests, then they will receive a failure notice and they will know that they have to fix it.

cpplint is a linter, so it doesn't really check formatting, it just basically checks codes for bugs, with some being related to style as well. That's why we need to run both cpplint and clang-format (see instructions for Eclipse; Clang-format already allows the Google style as one of its available styles, we just need to adjust the line length limit). I'll also look into what Jenkins allows, although I think I don't have the rights to see the jenkins configs we use for SGpp, so... 😒

@spc90 spc90 mentioned this issue Feb 3, 2020
Merged
@MichaelReh
Copy link
Member

The style is now mentioned in the developer guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@spc90 spc90

@leiterrl leiterrl

Labels
discussion General problem for which possible solutions need to be discussed feature request Desirable, nice-to-have feature
Milestone
No milestone
Development

No branches or pull requests
3 participants
@MichaelReh @spc90 @leiterrl
0