Skip to content

uncrustify-riot.cfg: update to coding convention#16149

Merged
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
maribu:uncrustify-col-length
Mar 8, 2021
Merged

uncrustify-riot.cfg: update to coding convention#16149
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
maribu:uncrustify-col-length

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Mar 4, 2021

Contribution description

The coding convention was changed to suggest breaking lines with with
more than 100 chars. This brings the uncrustify configuration back in
sync.

Testing procedure

I don't think this needs testing.

Issues/PRs references

Change of coding convention was done in #15793

The coding convention was changed to suggest breaking lines with with
more than 100 chars. This brings the uncrustify configuration back in
sync.
@maribu maribu added Area: tools Area: Supplementary tools Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Mar 4, 2021
@maribu maribu requested a review from kaspar030 March 4, 2021 16:27
@maribu maribu added 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 and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 5, 2021
@kaspar030
Copy link
Copy Markdown
Contributor

I guess we should apply the config to the uncrustify whitelist?

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Mar 5, 2021

If the files were already uncrustified with 80 char lines, there should be no change if we bump the max allowed length to 100.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Mar 6, 2021

I guess we should apply the config to the uncrustify whitelist?

I read this as: uncrustify should also check the uncrustify config. Is that correct?

When run locally, uncrustify checks the config as if it was C code and the result looks like modern art :-(

@kaspar030
Copy link
Copy Markdown
Contributor

I read this as: uncrustify should also check the uncrustify config. Is that correct?

no, as ben understood it, I suggested re-applying the new config to everything whitelisted for uncrustify.
Maybe I'm expecting too much, rustfmt would reformat lines that were previously cut at 80chars to what's the current setting.
Not sure uncrustify actually does that, and re-running it would change file formatting.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Mar 6, 2021

I'm almost certain that uncrustify doesn't touch line breaks unless lines are over the char limit. I often break lines when uncrustufy demands it, but in more readable way. So far, uncrustify always accepted my alternative line breaking. From that, it seems most plausible that uncrustify is pretty trivial and really only insert line breaks when lines are too long, but never removes them.

@fjmolinas
Copy link
Copy Markdown
Contributor

Is this still waiting for something?

@kaspar030
Copy link
Copy Markdown
Contributor

ACK.

@kaspar030 kaspar030 merged commit 9c78a45 into RIOT-OS:master Mar 8, 2021
@maribu maribu deleted the uncrustify-col-length branch March 8, 2021 09:44
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Mar 8, 2021

Thanks :-)

@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
@kaspar030 kaspar030 added the Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. label Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tools Area: Supplementary tools 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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants