AStyle addition to travis #6590

0xc0170 · 2018-04-10T13:58:12Z

Description

This is a new feature - be on master to check the code and be part of Mbed OS 5.9 when most of our codebase actually is conforming to the style.

Checking only files that has changed within the pull request. More PR will come later to start updating the code base

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

0xc0170 · 2018-04-11T08:43:26Z

travis-ci/astyle — Passed, 2 warnings

See travis status, info about how many files changed within this pull request are not conforming to our code style. I added 2 files changed for showcase, will revert once this is approved.
AStyle only checks files changed and suggests them to be corrected.

With this PR , we should be able to add this check and start checking the incoming files for code style. And starting updating our user facing API to conform (few more PR will come)

0xc0170 · 2018-04-11T08:53:15Z

drivers/AnalogIn.cpp

@@ -25,3 +25,6 @@ SingletonPtr<PlatformMutex> AnalogIn::_mutex;
 };

 #endif
+


These changes are only to showcase that the script is running properly - only changed files are being checked.

Will be removed prior integration

bcostm · 2018-04-11T12:54:11Z

Checking only files that has changed within the pull request.

Good idea but we need a way to skip folders and files from this check. Typically for ST, all the HAL driver files that we copy/paste from ST Cube packages. And the coding style in those files is different from the one in mbed...

0xc0170 · 2018-04-11T13:28:19Z

@bcostm The ignore folders are defined in https://github.com/ARMmbed/mbed-os/pull/6590/files#diff-4ed9365cd7635d05b5f90eab986b7a93 (astyleignore file). At the moment targets folder is excluded from the check thus wont affect any HAL/drivers.

The rule at the moment is - is file changed within the PR + is it not in astyle ignore file + is it h/c/cpp/hpp then it is checked

geky · 2018-04-11T16:05:16Z

.travis.yml

+      - astyle --version
+      script:
+        # only changed files to be checked for astyle
+        - git diff --name-only $TRAVIS_BRANCH | grep '.*\.\(h\|c\|hpp\|cpp\)' | fgrep -v -f .astyleignore | xargs -n 100 -I {} bash -c "astyle -n --options=.astylerc \"{}\"" > astyle.out;


So master will always say "Passed, 0 warnings".

We could also run the astyle branch on the current commit without printing the warnings so the status says "Passed, 204 warnings" on master. On the PR we can make it say "Passed, 204 warnings (+3 warnings)".

Is that necessary ? Master files are going to be cleaned up all of them (once this is approved, I'll start module by module). Thus master should be always 0. Only PR that modifies the files can be >0. Or?

What is the suggestion ?

The problem is this code will say "Passed, 0 warnings" even if there are 100 warnings.

More PR will come later to start updating the code base

So we don't have 0 warnings yet. Am I wrong?

If we merge this your future PRs aren't going to be very impressive. What will they say? "I removed 50 astyle warnings, reducing the total from 0 warnings to 0 warnings"? : )

I see your point. This is the first step, master has warnings now.
Your first proposal would be fine. We have a baseline - master - lets say 205 warnings. If PR introduces new ones, would do (+ 10 warnings). That would work until we get 0 on master ! I'll look at changes needed

Still not completely certain about the solution but would it be:

we do astyle on master branch

we do astyle on changes (as it is now here)

we show these : master + PR astyle warnings (+/- only PR astyle warnings)

Is that correct?

Oh! There status script will grab whatever number you have on master and automatically compare with that.

The only thing that needs to change is that CURR is set to the the total number of warnings.

So you probably just need to run astyle again without the changes.

Note we don't need to print out all of the warnings, the diff is more useful.

geky · 2018-04-11T16:06:26Z

.travis.yml

+          if [ "$PREV" -ne 0 ]
+          then
+              STATUSM="$STATUSM ($(python -c "print '%+d' % ($CURR-$PREV)") warnings)"
+          fi


As is, once master has an astyle status, PRs will say "Passed, 4 warnings (+4 warnings)". We should probably remove this script from here if we don't want to compare with master.

Why would we not want to compare with master? I would think that having warning delta would be useful, and part of this PR's intent.

See above comment for what's needed to compare with master. As is this is currently broken. (The "+4 warnings" is not a comparison)

cmonr · 2018-04-11T18:44:47Z

.travis.yml

+          if [ "$PREV" -ne 0 ]
+          then
+              STATUSM="$STATUSM ($(python -c "print '%+d' % ($CURR-$PREV)") warnings)"
+          fi


Why would we not want to compare with master? I would think that having warning delta would be useful, and part of this PR's intent.

cmonr · 2018-04-11T18:47:59Z

.astylerc

@@ -0,0 +1,34 @@
+# Mbed OS code style definition file for astyle


Asking for clarification. Some of these flags and comments sound like they modify the files themselves. Is this actually the case?

modify the files? can you elaborate?

Do flags such as convert-tabs and indent-cases modify files?

Yes, they do as any other flags there. astyle runs, changes files according to this style definition.

0xc0170 · 2018-04-13T09:06:33Z

@geky Added another step in after_success . Run astyle on the branch and that one used to compare with master, review please

geky · 2018-04-16T12:52:00Z

@0xc0170!

0xc0170 · 2018-04-16T14:42:17Z

Looks good to me ! +3 warnings will be eliminated once I remove those changes (should be done prior CI stage).

This follows what is in the codebase in most cases (K&R with few exceptions). This should follow also how online compiler formats the code.

Fetch 3.1 astyle from website (install from ubuntu contains old 2.05). Print version to verify and run on our codebase

This should reflect these rules that we have defined since mbed 2: ``` Indentation - 4 spaces. Please do not use tabs. Braces - K&R (see the exception 1 TBS below) 1 TBS -use braces for statements if, else, while, for (exception from K&R) Reference: http://en.wikipedia.org/wiki/Indent_style#Variant:_1TBS One line per statement Preprocessor macro starts at the beginning of a new line, the code inside is indented accordingly the code above it Cases within switch are indented (exception from K&R) Space after statements if, while, for, switch, same applies to binary and ternary operators Each line has preferably at most 120 characters For pointers, '*' is adjacent to a data name (analogin_t *obj) or a function name (analog_t *get_analogin_object()) Don't leave trailing spaces at the end of lines Empty lines should have no trailing spaces Unix line endings are default option for files Use capital letters for macros A file should have an empty line at the end ```

To compare to master, use entire branch, not only files changed.

0xc0170 · 2018-04-19T11:56:47Z

Rebased (latest master) and removed the changes that were testing the script (visible output above in the screen shot #6590 (comment)).

time to test this one

0xc0170 · 2018-04-19T11:57:38Z

/morph build

mbed-ci · 2018-04-19T12:45:02Z

Build : SUCCESS

Build number : 1802
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6590/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-19T18:40:47Z

Exporter Build : SUCCESS

Build number : 1450
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6590/

mbed-ci · 2018-04-19T22:22:48Z

Test : SUCCESS

Build number : 1611
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6590/1611

0xc0170 added the do not merge label Apr 10, 2018

0xc0170 changed the title ~~AStyle~~ AStyle addition to travis Apr 10, 2018

0xc0170 force-pushed the dev_astyle_only_changed branch 10 times, most recently from 4e76b2a to 5da1615 Compare April 11, 2018 07:46

0xc0170 requested review from geky, cmonr and screamerbg April 11, 2018 08:44

0xc0170 added needs: review and removed do not merge labels Apr 11, 2018

0xc0170 mentioned this pull request Apr 11, 2018

Add AStyle definition file and travis astyle check #6014

Closed

0xc0170 commented Apr 11, 2018

View reviewed changes

geky previously approved these changes Apr 11, 2018

View reviewed changes

geky self-requested a review April 11, 2018 16:06

cmonr reviewed Apr 11, 2018

View reviewed changes

0xc0170 requested a review from sg- April 13, 2018 08:40

0xc0170 dismissed geky’s stale review via b264218 April 13, 2018 09:00

0xc0170 force-pushed the dev_astyle_only_changed branch from b264218 to 5f87fe6 Compare April 13, 2018 09:01

0xc0170 force-pushed the dev_astyle_only_changed branch from 5f87fe6 to e894365 Compare April 13, 2018 11:07

0xc0170 force-pushed the dev_astyle_only_changed branch 3 times, most recently from cefc2c2 to b48ff35 Compare April 13, 2018 11:59

0xc0170 and others added 8 commits April 19, 2018 12:55

Add Astyle configuration file for mbed OS

f07cf4c

This follows what is in the codebase in most cases (K&R with few exceptions). This should follow also how online compiler formats the code.

Add astyleignore file

1ff77d8

Travis: add astyle

f6541f9

Fetch 3.1 astyle from website (install from ubuntu contains old 2.05). Print version to verify and run on our codebase

Astyle: Changed from test failure to status note

396fa5f

travis: astyle only on changed files in PR

2cd0b81

travis: fix commit content

6d427ea

travis: add astyle run for all changes in the PR

c12ee10

To compare to master, use entire branch, not only files changed.

0xc0170 force-pushed the dev_astyle_only_changed branch from b48ff35 to c12ee10 Compare April 19, 2018 11:56

AnotherButler mentioned this pull request Apr 24, 2018

AStyle update (use options and ignore file for astyle) ARMmbed/mbed-os-5-docs#400

Merged

screamerbg approved these changes Apr 27, 2018

View reviewed changes

sg- approved these changes Apr 27, 2018

View reviewed changes

cmonr approved these changes Apr 27, 2018

View reviewed changes

cmonr added ready for merge release-version: 5.9.0-rc1 and removed needs: review labels Apr 27, 2018

geky approved these changes Apr 27, 2018

View reviewed changes

cmonr merged commit a1307d4 into ARMmbed:master Apr 30, 2018

cmonr removed the ready for merge label Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AStyle addition to travis #6590

AStyle addition to travis #6590

0xc0170 commented Apr 10, 2018 •

edited

Loading

0xc0170 commented Apr 11, 2018

0xc0170 Apr 11, 2018 •

edited

Loading

bcostm commented Apr 11, 2018

0xc0170 commented Apr 11, 2018 •

edited

Loading

geky Apr 11, 2018

0xc0170 Apr 11, 2018 •

edited

Loading

geky Apr 11, 2018 •

edited

Loading

0xc0170 Apr 12, 2018

0xc0170 Apr 12, 2018

geky Apr 12, 2018

geky Apr 11, 2018

cmonr Apr 11, 2018

geky Apr 11, 2018

cmonr Apr 11, 2018

cmonr Apr 11, 2018

0xc0170 Apr 11, 2018

cmonr Apr 12, 2018

0xc0170 Apr 13, 2018

0xc0170 commented Apr 13, 2018

geky commented Apr 16, 2018

0xc0170 commented Apr 16, 2018

0xc0170 commented Apr 19, 2018

0xc0170 commented Apr 19, 2018

mbed-ci commented Apr 19, 2018

mbed-ci commented Apr 19, 2018

mbed-ci commented Apr 19, 2018

		@@ -25,3 +25,6 @@ SingletonPtr<PlatformMutex> AnalogIn::_mutex;
		};

		#endif

		@@ -0,0 +1,34 @@
		# Mbed OS code style definition file for astyle

AStyle addition to travis #6590

AStyle addition to travis #6590

Conversation

0xc0170 commented Apr 10, 2018 • edited Loading

Description

Pull request type

0xc0170 commented Apr 11, 2018

0xc0170 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

bcostm commented Apr 11, 2018

0xc0170 commented Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

0xc0170 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

geky Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Apr 13, 2018

geky commented Apr 16, 2018

0xc0170 commented Apr 16, 2018

0xc0170 commented Apr 19, 2018

0xc0170 commented Apr 19, 2018

mbed-ci commented Apr 19, 2018

Triggering tests

mbed-ci commented Apr 19, 2018

mbed-ci commented Apr 19, 2018

0xc0170 commented Apr 10, 2018 •

edited

Loading

0xc0170 Apr 11, 2018 •

edited

Loading

0xc0170 commented Apr 11, 2018 •

edited

Loading

0xc0170 Apr 11, 2018 •

edited

Loading

geky Apr 11, 2018 •

edited

Loading