[Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/automate_clang-format into lp:widelands has been updated. Status: Needs review => Merged For more details, see: https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/automate_clang-format. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
Thanks for the review! So, pyformat doesn't fix long lines, need to keep that in mind. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/automate_clang-format. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
Review: Approve
pretty hard to review this diff with the formatting changes. I found two nits,
I think otherwise this is ready to merge.
Diff comments:
> === renamed file 'utils/fix_lua_tabs.py' => 'utils/fix_formatting.py'
> --- utils/fix_lua_tabs.py 2015-10-31 12:11:44 +
> +++ utils/fix_formatting.py 2016-12-03 18:02:09 +
> @@ -2,29 +2,35 @@
> # -*- coding: utf-8 -*-
>
>
> -"""The code base had inconsistent usage of tabs/spaces for indenting in Lua
> +"""The code base had inconsistent usage of tabs/spaces for indenting in Lua.
> +
> files. Spaces were more prominent - and I prefer them over tabs. So I wrote
> this small script to fix leading tabs in Lua files to spaces.
>
> It also saves files in unix file endings ("\r\n") and strips empty lines at
> the
> end of files and whitespace characters at the end of lines.
> +
> +After fixing the Lua tabs, this script also executes clang-format over the
> src directory and pyformat over the utils directory.
overlong line.
> +
> """
>
> import argparse
> import os
> import re
> import sys
> +from subprocess import call
>
> LEADING_TABS = re.compile(r'^\s*\t+\s*')
> PYTHON3 = sys.version_info >= (3, 0)
> SPACES_PER_TAB = 3
>
> +
> def parse_args():
> -p = argparse.ArgumentParser(description=
> -"Fix common whitespace errors in Lua files. Recurses over all Lua
> files."
> -)
> +p = argparse.ArgumentParser(
> +description='Fix common whitespace errors in Lua files, run
> clang-format over the code base and pyformat over the utils directory.
> Recurses over all relevant files.')
overlong line. You can split strings in python line in c: "hello world" is the
same as "hello"
" world"
> return p.parse_args()
>
> +
> def read_text_file(filename):
> """Reads the contens of a text file."""
> if PYTHON3:
--
https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/automate_clang-format.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/automate_clang-format into lp:widelands has been updated. Status: Needs review => Work in progress For more details, see: https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/automate_clang-format. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
Thanks - all fixed. I was just thinking - we also have fix_lua_tabs.py, which is run when we update the translations. How about having 1 central script "fix_formating.py" that does the following: - fix_lua_tabs.py - run_clang_format.py - run pyformat over utils. This script could then be called both by merge_and_push_translations.sh and bunnybot. -- https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/automate_clang-format. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
> Can this be added to the bunnybot merge command once we have this in trunk?
> I'm thinking bzr merge, clang-format, bzr commit here.
Yes, it probably can. If I can figure out how to install clang-format on the
buildbot. I'll have a look.
A couple of nits inlined.
Diff comments:
> === added file 'utils/run_clang_format.py'
> --- utils/run_clang_format.py 1970-01-01 00:00:00 +
> +++ utils/run_clang_format.py 2016-12-01 18:07:34 +
> @@ -0,0 +1,42 @@
> +#!/usr/bin/env python
> +# -*- coding: utf-8 -*-
> +
> +
> +"""This script runs clang-format over src and all its subdirectories."""
> +
> +import argparse
> +import os
> +import sys
> +from subprocess import call
> +
> +
> +def parse_args():
> +p = argparse.ArgumentParser(description='Run clang-format over the code
> base.'
move closing ) up?
> +)
> +return p.parse_args()
> +
> +
> +def find_cplusplus_files():
> +for (dirpath, _, filenames) in os.walk('./src'):
> +for filename in filenames:
> +if os.path.splitext(filename)[-1].lower() == '.cc' or
> os.path.splitext(filename)[-1].lower() == '.h':
if os.path.splitext(filename)[-1].lower() in ['.cc', '.h']
> +yield os.path.join(dirpath, filename)
> +
> +
> +def main():
> +parse_args()
> +
> +if not os.path.isdir('src') or not os.path.isdir('utils'):
> +print('CWD is not the root of the repository.')
> +return 1
> +
> +sys.stdout.write('Running clang-format ')
> +for filename in find_cplusplus_files():
> +sys.stdout.write('.')
> +sys.stdout.flush()
> +call(['clang-format', '-i', filename])
> +print '\nFormatting finished.'
> +return 0
> +
> +if __name__ == '__main__':
> +sys.exit(main())
--
https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/automate_clang-format.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
Continuous integration builds have changed state: Travis build 1679. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/180479785. Appveyor build 1519. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_automate_clang_format-1519. -- https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/automate_clang-format. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/automate_clang-format into lp:widelands
GunChleoc has proposed merging
lp:~widelands-dev/widelands/automate_clang-format into lp:widelands.
Commit message:
Added script to utils for running clang-format over the code base.
Requested reviews:
SirVer (sirver)
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/automate_clang-format/+merge/312287
I think it would be a good idea to automate clang-format - it is starting to
cost me real time when I work on a branch and trunk hasn't been formatted
properly.
Can this be added to the bunnybot merge command once we have this in trunk? I'm
thinking bzr merge, clang-format, bzr commit here.
--
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/automate_clang-format.
=== added file 'utils/run_clang_format.py'
--- utils/run_clang_format.py 1970-01-01 00:00:00 +
+++ utils/run_clang_format.py 2016-12-01 18:04:25 +
@@ -0,0 +1,43 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+
+"""This script runs clang-format over src and all its subdirectories."""
+
+import argparse
+import os
+import sys
+from subprocess import call
+
+
+def parse_args():
+p = argparse.ArgumentParser(description='Run clang-format over the code base.'
+)
+return p.parse_args()
+
+
+def find_cplusplus_files():
+for (dirpath, _, filenames) in os.walk('./src'):
+for filename in filenames:
+if os.path.splitext(filename)[-1].lower() == '.cc' or os.path.splitext(filename)[-1].lower() == '.h':
+yield os.path.join(dirpath, filename)
+
+
+def main():
+parse_args()
+
+if not os.path.isdir('src') or not os.path.isdir('utils'):
+print('CWD is not the root of the repository.')
+return 1
+
+sys.stdout.write('Running clang-format ')
+for filename in find_cplusplus_files():
+# print "Formatting %r" % filename
+sys.stdout.write('.')
+sys.stdout.flush()
+call(['clang-format-3.8', '-i', filename])
+print '\nFormatting finished.'
+return 0
+
+if __name__ == '__main__':
+sys.exit(main())
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
