packages: add option to suppress warning messages printed directly to console #114
Conversation
|
The changes look reasonable, but there is a reason the warnings are being printed... For building locally, I think this is a valid scenario for ignoring warnings like this. It would be nice to get the warnings rather than only having them printed to the screen or completely avoided. Maybe rather than |
| @@ -357,18 +359,19 @@ def parse_package(path): | |||
|
|
|||
| with open(filename, 'r') as f: | |||
| try: | |||
| return parse_package_string(f.read(), filename) | |||
| return parse_package_string(f.read(), filename, suppress_warnings) | |||
There was a problem hiding this comment.
Please pass kwargs as kwargs instead of positional. Same below.
|
Very good point from William, changing the new parameter to "collect" the warnings would be even better. |
Yeah, I'm down. |
jbohren
force-pushed
the
suppress-warnings
branch
from
7423e36
to
c9dfa9b
Compare
|
Ok. c9dfa9b adds warnings to a list passed into the |
|
Other than the comment, lgtm. |
jbohren
force-pushed
the
suppress-warnings
branch
from
c9dfa9b
to
4c507e0
Compare
| if warnings is None: | ||
| print('WARNING: ' + warning, file=sys.stderr) | ||
| elif warning not in warnings: | ||
| warnings += warning |
There was a problem hiding this comment.
If warnings is a list and warning is a string than this operation won't do the right thing?
|
Can you please add a unit test for this new feature to ensure it works as expected. |
jbohren
force-pushed
the
suppress-warnings
branch
from
4c507e0
to
0ead4ca
Compare
|
Yeah I'll add a test. |
jbohren
force-pushed
the
suppress-warnings
branch
from
0ead4ca
to
0f26996
Compare
|
Test added. |
| pkgs_dict = find_packages(test_data_dir, warnings=warnings) | ||
|
|
||
| for w, we in zip(warnings, test_expected_warnings): | ||
| assert(w == we) |
There was a problem hiding this comment.
You should try to give a second argument to assert when ever you use it, because this line is failing on the farm, but I don't know why exactly.
You can also just do self.assertEqual(sorted(w), sorted(we)), or if you are certain what the order should be self.assertEqual(w, we). The self.assertEqual will automatically give you a nice comparison out if it fails.
There was a problem hiding this comment.
Yeah, I'll get it working on the farm.
jbohren
force-pushed
the
suppress-warnings
branch
3 times, most recently
from
3c2bf26
to
994db97
Compare
|
lgtm, @dirk-thomas? |
| @@ -357,18 +367,19 @@ def parse_package(path): | |||
|
|
|||
| with open(filename, 'r') as f: | |||
| try: | |||
| return parse_package_string(f.read(), filename) | |||
| return parse_package_string(f.read(), filename, warnings) | |||
There was a problem hiding this comment.
Please always pass kwargs with the explicit keyword instead of relying on the position. Same below.
jbohren
force-pushed
the
suppress-warnings
branch
2 times, most recently
from
02238fd
to
6e8adca
Compare
|
Ready for review again. |
| @@ -529,7 +540,7 @@ def parse_package_string(data, filename=None): | |||
| if errors: | |||
| raise InvalidPackage('Error(s) in %s:%s' % (filename, ''.join(['\n- %s' % e for e in errors]))) | |||
|
|
|||
| pkg.validate() | |||
| pkg.validate(warnings) | |||
There was a problem hiding this comment.
Last comment - promise 😉
This one should use the keyword too: warnings=warnings.
jbohren
force-pushed
the
suppress-warnings
branch
from
6e8adca
to
34d49ea
Compare
|
Ready again again. |
|
Thank you for making all the changes for the numerous small comments. LGTM. |
packages: add option to suppress warning messages printed directly to console
`catkin_generate_changelog` outputs changelog with what we called with `--skip-merge` option, and reason is that `git show` does not show files for merged commit, we need '-m' option
```
k-okada@p40-yoga:/tmp/jsk_3rdparty$ git show 6a0bfaaf29bcc9a8e07e6edfccd230ad7f003070 --name-only --format=format:""
k-okada@p40-yoga:/tmp/jsk_3rdparty$ git show 6a0bfaaf29bcc9a8e07e6edfccd230ad7f003070
commit 6a0bfaaf29bcc9a8e07e6edfccd230ad7f003070
Merge: 5f6888c 54ea9bd
Author: Kei Okada <k-okada@jsk.t.u-tokyo.ac.jp>
Date: Fri Jul 7 11:32:32 2017 +0900
Merge pull request ros-infrastructure#114 from k-okada/add_unzip
add unzip to build_depend
```
* "--skip-merge" does not change any outputs
`catkin_generate_changelog` outputs changelog with what we called with `--skip-merge` option, and reason is that `git show` does not show files for merged commit, we need '-m' option
```
k-okada@p40-yoga:/tmp/jsk_3rdparty$ git show 6a0bfaaf29bcc9a8e07e6edfccd230ad7f003070 --name-only --format=format:""
k-okada@p40-yoga:/tmp/jsk_3rdparty$ git show 6a0bfaaf29bcc9a8e07e6edfccd230ad7f003070
commit 6a0bfaaf29bcc9a8e07e6edfccd230ad7f003070
Merge: 5f6888c 54ea9bd
Author: Kei Okada <k-okada@jsk.t.u-tokyo.ac.jp>
Date: Fri Jul 7 11:32:32 2017 +0900
Merge pull request #114 from k-okada/add_unzip
add unzip to build_depend
```
* use --first-parent, instead of -m
No description provided.