Skip to content

vera++: increase line length#15793

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/ci/vera_increase_line_length
Jan 19, 2021
Merged

vera++: increase line length#15793
miri64 merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/ci/vera_increase_line_length

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Jan 19, 2021

Contribution description

The vera++ warning annotation about the 80 character max line length is cluttering the review process on certain files.

This PR is increasing the warning line length to 100 and max line length to 120.

Testing procedure

The bogus commit should report annotations about maximum line length.

Issues/PRs references

Should improve the situation in PR like #15718 (see https://github.com/RIOT-OS/RIOT/pull/15718/files#diff-1e6a82d8739f75dd4cddea9ecc25505ed042f3670ee5646dfc0c97dba2881662)

@aabadie aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CI Area: Continuous Integration of RIOT components CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Jan 19, 2021
@aabadie aabadie requested a review from maribu January 19, 2021 10:58
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jan 19, 2021

The new values are correctly taken into account. Next question: do these values make sense ?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

If the coding conventions are changed, this should be documented as well ;-).

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

This PR is increasing the warning line length to 100 and max line length to 120.

Are we really needing 120 here? Why not just set he maximum line length to 100, period?

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Jan 19, 2021

I'm also for a single max. Having a warning line would trigger warning messages from Vera++ on every PR that changes a file with lines over the warning limit, even if the changes are not related to the lines causing the warning (right?)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

This also would make a mod of the coding conventions unnecessary. The optional 80 char max could still be upheld manually.

@aabadie aabadie requested a review from jia200x as a code owner January 19, 2021 12:30
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jan 19, 2021

If the coding conventions are changed, this should be documented as well

I pushed b5c7d9d to adapt the values in the coding conventions.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jan 19, 2021

Hmm, sorry I misunderstood the comments above. So we agree on a single value of 100 characters per line ?

@aabadie aabadie force-pushed the pr/ci/vera_increase_line_length branch from b5c7d9d to 65489f1 Compare January 19, 2021 12:35
miri64
miri64 reviewed Jan 19, 2021
View reviewed changes
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

Hmm, sorry I misunderstood the comments above. So we agree on a single value of 100 characters per line ?

As the absolute maximum, yes. I think however we can keep the 80 char limit as someone an individual developer can look out for or configure their IDE to ;-).

@aabadie aabadie force-pushed the pr/ci/vera_increase_line_length branch from 65489f1 to dc0b638 Compare January 19, 2021 12:44
miri64
miri64 approved these changes Jan 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK from my side. @jia200x if you like to comment, do it soon ;-).

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 19, 2021

I'm fine with it!

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 19, 2021
maribu
maribu approved these changes Jan 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

@aabadie please remove the REMOVE ME commit :-).

@aabadie aabadie force-pushed the pr/ci/vera_increase_line_length branch from dc0b638 to 8f2b375 Compare January 19, 2021 15:11
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jan 19, 2021

Another side effect of this PR will be to limit the output of the vera++ check when run on master.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

Another side effect of this PR will be to limit the output of the vera++ check when run on master.

I actually thought that was your main motivation 😀

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jan 19, 2021

I actually thought that was your main motivation

Let's say it was not the only one :)

@miri64 miri64 merged commit 039ede4 into RIOT-OS:master Jan 19, 2021
@aabadie aabadie deleted the pr/ci/vera_increase_line_length branch January 19, 2021 15:41
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 19, 2021

Mhh for some reason the 80 chars warning still shows up.

@jia200x jia200x mentioned this pull request Jan 20, 2021
Merged
@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 20, 2021

Mhh for some reason the 80 chars warning still shows up.

see #15813

@maribu maribu mentioned this pull request Mar 4, 2021
Merged
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
This was referenced Oct 26, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@maribu maribu maribu approved these changes

@jia200x jia200x Awaiting requested review from jia200x

Assignees

No one assigned

Labels

Area: CI Area: Continuous Integration of RIOT components CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2021.04

Development

Successfully merging this pull request may close these issues.

6 participants

@aabadie @miri64 @bergzand @jia200x @maribu @kaspar030