Expand ignores to pep257 definition. #241

Arnatious · 2020-05-12T00:10:55Z

Fixes #240 by using --add-ignore to allow user to expand pep257 ignores, and add error codes to check.

-add-select functionality added via the --select flag, allowing full customization of error codes to check

Signed-off-by: Ted Kern ted.kern@canonical.com

hidmic

I see the interest in changing ament_pep257 to actually comply with PEP257, but what's the harm in the former performing a few more checks?

Arnatious · 2020-05-14T18:38:34Z

I see the interest in changing ament_pep257 to actually comply with PEP257, but what's the harm in the former performing a few more checks?

At least in the case of D212 and D213, pep257 allows for docstrings to start either on the same line as the opening quotes or the next line. These error checks pigeonhole the user into strictly one style or the other, causing valid strings under pep257 to fail. The user should have to explicitly enable one to check for consistency if they care about that.

The remainder are fine to keep, though some are redundant.

ament_pep257/ament_pep257/main.py

kyrofa · 2020-05-15T20:25:01Z

ament_pep257/ament_pep257/main.py

+    elif args.allow_undocumented is None:
+        warnings.warn('Argument "--allow-undocumented" will be required in a future version. '
+                      'Defaulting to --allow-undocumented=True')
+        args.ignore.append('D1')


Thanks for keeping backward compatibility.

This is great as a long-term goal, but I do not know if there'll be resources to document all core ROS 2 python code in the near future. Meaning, the warning may result in unwarranted noise.

ament_pep257/ament_pep257/main.py

hidmic · 2020-06-03T20:43:13Z

ament_pep257/ament_pep257/main.py

+    elif args.allow_undocumented is None:
+        warnings.warn('Argument "--allow-undocumented" will be required in a future version. '
+                      'Defaulting to --allow-undocumented=True')
+        args.ignore.append('D1')


This is great as a long-term goal, but I do not know if there'll be resources to document all core ROS 2 python code in the near future. Meaning, the warning may result in unwarranted noise.

hidmic · 2020-06-03T20:45:47Z

ament_pep257/ament_pep257/main.py

@@ -45,11 +46,20 @@ def main(argv=sys.argv[1:]):
    parser.add_argument(
        '--ignore',
        nargs='*',
-        default=[
-            'D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D106', 'D107',
-            'D203', 'D212', 'D404',


These error checks pigeonhole the user into strictly one style or the other, causing valid strings under pep257 to fail.

I understand, but changing this tool defaults will affect the style that is enforced throughout ROS 2 core packages. D212 was intentionally introduced by #61, it wasn't an omission. @Karsten1987 do you recall the rationale behind that patch?

Ultimately, an external user could relax constraints (i.e. ignore both D212 and D213) without us changing the defaults.

Restored the default error checking behavior in e09982a.

Note that some merging code is necessary to duplicate the old behavior

ament_lint/ament_pep257/ament_pep257/main.py

Lines 76 to 82 in e09982a

default_ignore = set(['D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D106', 'D107'])

if 'D1' in args.select:

default_ignore = set([])

default_ignore -= set(args.select)

args.ignore += default_ignore

args.select.append('D213')

Hmm, instead of making assumptions on what the default ignored error codes are and forcing others to set a baseline, I'd much rather replicate pydocstyle interface i.e. to have --ignore, --select, --add-ignore, and --add-select, and keep the default --ignore list as it used to be. Thoughts?

hidmic · 2020-06-11T19:37:43Z

@Arnatious friendly ping.

sloretz · 2020-06-25T16:41:46Z

@Arnatious @hidmic friendly ping :) It looks like @Arnatious has made changes since your review, and now there's a merge conflict with this PR.

@Arnatious mind fixing the merge conflict?

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious · 2020-06-25T17:21:15Z

@sloretz rebased onto latest master. @hidmic ready for review

Signed-off-by: Ted Kern <ted.kern@canonical.com>

hidmic

LGTM but for a few comments

hidmic · 2020-07-03T18:09:45Z

ament_pep257/ament_pep257/main.py

+        '--convention',
+        choices=pydocstyle.conventions,
+        default='ament',
+        help=f'Choose a preset list of error codes. Valid options are {pydocstyle.conventions}'


@Arnatious will ament show up in pydocstyle.conventions? I'd expect ament_pep257 --convention=ament not to fail, even if redundant.

a4d6399 added logic to populate/allow "ament" as an option

hidmic · 2020-07-03T18:13:40Z

ament_pep257/ament_pep257/main.py

+                                'D107',
+                                'D203',
+                                'D212',
+                                'D404'])


@Arnatious uh, is flake8 happy with this?

It was, but I ran it through black in 6a645fc and it looks better now

Signed-off-by: Ted Kern <ted.kern@canonical.com>

hidmic

LGTM

hidmic · 2020-07-06T18:34:54Z

@Arnatious mind to fix DCO? I'll run CI afterwards.

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious · 2020-07-06T18:40:07Z

@hidmic fixed

dirk-thomas · 2020-07-06T18:44:17Z

How can a user get the information what codes are being ignored (e.g. for the ament convention) without reading the source code?

hidmic · 2020-07-06T18:58:32Z

Running CI up to rclpy:

Linux
Linux-aarch64
macOS
Windows

Arnatious · 2020-07-06T19:06:15Z

How can a user get the information what codes are being ignored (e.g. for the ament convention) without reading the source code?

@dirk-thomas that's a good question. --convention is taken right from pydocstyle, where the only documentation of those error codes is at http://www.pydocstyle.org/en/latest/error_codes.html#default-conventions .

ament is pep257, with the addition of ignoring "D1" (undocumented) and for some unexplained reason enforcing D213.

Would a readme explaining the options coming from pydocstyle/linking to the pydocstyle style docs as appropriate be desired?

dirk-thomas · 2020-07-06T19:14:47Z

Would a readme explaining the options coming from pydocstyle/linking to the pydocstyle style docs as appropriate be desired?

Before this patch a user could call ament_pep257 --help to get the information which I think is the most intuitive way (at least for the ament convention).

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious · 2020-07-06T19:32:31Z

@dirk-thomas added help text in e96b4ab

Resulting output is

usage: ament_pep257 [-h] [--ignore IGNORE [IGNORE ...] | --select SELECT
                    [SELECT ...] | --convention {google,pep257,numpy,ament}]
                    [--add-ignore ADD_IGNORE [ADD_IGNORE ...]]
                    [--add-select ADD_SELECT [ADD_SELECT ...]]
                    [--exclude [filename [filename ...]]]
                    [--xunit-file XUNIT_FILE]
                    [paths [paths ...]]

Check docstrings against the style conventions in PEP 257.

positional arguments:
  paths                 The files or directories to check. For directories,
                        files ending in '.py' will be considered. (default:
                        ['.'])

optional arguments:
  -h, --help            show this help message and exit
  --ignore IGNORE [IGNORE ...]
                        Choose the list of error codes for pydocstyle NOT to
                        check for. (default: [])
  --select SELECT [SELECT ...]
                        Choose the basic list of error codes for pydocstyle to
                        check for. (default: [])
  --convention {google,pep257,numpy,ament}
                        Choose a preset list of error codes. Valid options are
                        {'google', 'pep257', 'numpy', 'ament'}. The "ament"
                        convention is defined as --ignore ['D100', 'D101',
                        'D102', 'D103', 'D104', 'D105', 'D106', 'D107',
                        'D203', 'D212', 'D404']. (default: ament)
  --add-ignore ADD_IGNORE [ADD_IGNORE ...]
                        Ignore an extra error code, removing it from the list
                        set by --(select/ignore) (default: [])
  --add-select ADD_SELECT [ADD_SELECT ...]
                        Check an extra error code, adding it to the list set
                        by --(select/ignore). (default: [])
  --exclude [filename [filename ...]]
                        The filenames to exclude. (default: [])
  --xunit-file XUNIT_FILE
                        Generate a xunit compliant XML file (default: None)

dirk-thomas · 2020-07-06T19:36:56Z

added help text in e96b4ab

Thanks, LGTM.

hidmic · 2020-07-14T13:05:32Z

Alright, CI's green, PR's approved. Going in. Thanks for your patience @Arnatious !

emersonknapp · 2020-07-14T18:58:19Z

This PR seems to be causing runtime errors in our builds - e.g. https://github.com/ros-tooling/action-ros-ci/runs/870259711?check_suite_focus=true

  [3.105s] test/test_pep257.py F                                                    [100%]
  [3.105s] 
  [3.105s] =================================== FAILURES ===================================
  [3.105s] _________________________________ test_pep257 __________________________________
  [3.105s] test/test_pep257.py:22: in test_pep257
  [3.106s]     rc = main(argv=[])
  [3.106s] ../../../../build/ament_pep257/ament_pep257/main.py:69: in main
  [3.106s]     help='Choose the list of error codes for pydocstyle NOT to check for.')
  [3.106s] /usr/lib/python3.6/argparse.py:1346: in add_argument
  [3.106s]     raise ValueError('unknown action "%s"' % (action_class,))
  [3.106s] E   ValueError: unknown action "extend"

I'm not sure exactly what this issue is, but the extend action came from this PR

emersonknapp · 2020-07-14T19:00:10Z

Followup - the extend action is new in Python 3.8, but ROS 2 Foxy supports Python 3.7 as per REP-2000 https://www.ros.org/reps/rep-2000.html#foxy-fitzroy-may-2020-may-2023

My recommendation is to revert this PR and then make sure we have a Python3.7-compliant build before merging it again.

kyrofa · 2020-07-14T19:00:20Z

Indeed, looks like extend is new of as 3.8 (https://docs.python.org/3/library/argparse.html). @Arnatious mind fixing that?

Arnatious · 2020-07-14T23:05:48Z

The behavior of extend isn't actually necessary, so I removed it in 7623ada, my bad @emersonknapp

emersonknapp · 2020-07-14T23:08:19Z

Thanks for the fix - is that commit in a followup PR?

Arnatious · 2020-07-14T23:21:16Z

Thanks for the fix - is that commit in a followup PR?

Yes, #262

Arnatious changed the title ~~Expand ignores to pep257 definition. (ament #240)~~ Expand ignores to pep257 definition. May 12, 2020

This was referenced May 12, 2020

Index api ubuntu-robotics/nodl#11

Merged

remove pep257 check pending upstream fixes ubuntu-robotics/nodl#12

Merged

hidmic reviewed May 14, 2020

View reviewed changes

kyrofa reviewed May 15, 2020

View reviewed changes

ament_pep257/ament_pep257/main.py Outdated Show resolved Hide resolved

kyrofa reviewed May 15, 2020

View reviewed changes

ament_pep257/ament_pep257/main.py Outdated Show resolved Hide resolved

kyrofa reviewed May 15, 2020

View reviewed changes

ament_pep257/ament_pep257/main.py Outdated Show resolved Hide resolved

Arnatious force-pushed the arnatious/pep257_error_codes branch from 0b9856b to 9917409 Compare May 15, 2020 20:29

sloretz requested a review from hidmic May 29, 2020 03:27

hidmic reviewed Jun 3, 2020

View reviewed changes

sloretz assigned hidmic Jun 11, 2020

Arnatious force-pushed the arnatious/pep257_error_codes branch from 9917409 to 3ad9a2e Compare June 12, 2020 00:58

Ted Kern added 3 commits June 25, 2020 17:20

Expand ignores to pep257 definition. (ament ament#240)

f83313f

Signed-off-by: Ted Kern <ted.kern@canonical.com>

add '--allow-undocumented' flag to enforce pep257

4b80cce

Signed-off-by: Ted Kern <ted.kern@canonical.com>

restore existing default error codes to check

555b41d

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious force-pushed the arnatious/pep257_error_codes branch from e09982a to 555b41d Compare June 25, 2020 17:20

Arnatious force-pushed the arnatious/pep257_error_codes branch 2 times, most recently from b75c8db to b307f10 Compare June 25, 2020 18:06

fix no-ignores logic

c59cc3d

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious force-pushed the arnatious/pep257_error_codes branch from b307f10 to c59cc3d Compare June 25, 2020 18:29

Arnatious requested a review from hidmic June 25, 2020 18:32

expose options from pydocstyle

600f539

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious force-pushed the arnatious/pep257_error_codes branch from 9150dd4 to 600f539 Compare June 29, 2020 23:02

hidmic reviewed Jul 3, 2020

View reviewed changes

allow user to explicitly set convention to "ament"

a4d6399

Signed-off-by: Ted Kern <ted.kern@canonical.com>

fix typo in populating argv for pydocstyle

1fa8013

Signed-off-by: Ted Kern <ted.kern@canonical.com>

hidmic approved these changes Jul 6, 2020

View reviewed changes

reformat ament convention list

f5404f1

Signed-off-by: Ted Kern <ted.kern@canonical.com>

Arnatious force-pushed the arnatious/pep257_error_codes branch from 6a645fc to f5404f1 Compare July 6, 2020 18:39

Add help info for ament convention

e96b4ab

Signed-off-by: Ted Kern <ted.kern@canonical.com>

hidmic merged commit 5ee7e83 into ament:master Jul 14, 2020

emersonknapp mentioned this pull request Jul 14, 2020

Check out the correct ros2 sources for a given ROS 2 distribution ros-tooling/action-ros-ci#269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand ignores to pep257 definition. #241

Expand ignores to pep257 definition. #241

Arnatious commented May 12, 2020 •

edited

Loading

hidmic left a comment

Arnatious commented May 14, 2020 •

edited

Loading

kyrofa May 15, 2020 •

edited

Loading

hidmic Jun 3, 2020 •

edited

Loading

hidmic Jun 3, 2020 •

edited

Loading

hidmic Jun 3, 2020

hidmic Jun 3, 2020

Arnatious Jun 12, 2020

hidmic Jun 29, 2020

hidmic commented Jun 11, 2020

sloretz commented Jun 25, 2020

Arnatious commented Jun 25, 2020

hidmic left a comment •

edited

Loading

hidmic Jul 3, 2020

Arnatious Jul 5, 2020

hidmic Jul 3, 2020

Arnatious Jul 5, 2020

hidmic left a comment

hidmic commented Jul 6, 2020

Arnatious commented Jul 6, 2020

dirk-thomas commented Jul 6, 2020

hidmic commented Jul 6, 2020 •

edited

Loading

Arnatious commented Jul 6, 2020

dirk-thomas commented Jul 6, 2020

Arnatious commented Jul 6, 2020

dirk-thomas commented Jul 6, 2020

hidmic commented Jul 14, 2020

emersonknapp commented Jul 14, 2020

emersonknapp commented Jul 14, 2020

kyrofa commented Jul 14, 2020 •

edited

Loading

Arnatious commented Jul 14, 2020

emersonknapp commented Jul 14, 2020 •

edited

Loading

Arnatious commented Jul 14, 2020

	default_ignore = set(['D100', 'D101', 'D102', 'D103', 'D104', 'D105', 'D106', 'D107'])
	if 'D1' in args.select:
	default_ignore = set([])
	default_ignore -= set(args.select)
	args.ignore += default_ignore

	args.select.append('D213')

Expand ignores to pep257 definition. #241

Expand ignores to pep257 definition. #241

Conversation

Arnatious commented May 12, 2020 • edited Loading

hidmic left a comment

Choose a reason for hiding this comment

Arnatious commented May 14, 2020 • edited Loading

kyrofa May 15, 2020 • edited Loading

Choose a reason for hiding this comment

hidmic Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

hidmic Jun 3, 2020 • 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

hidmic commented Jun 11, 2020

sloretz commented Jun 25, 2020

Arnatious commented Jun 25, 2020

hidmic left a comment • 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

hidmic left a comment

Choose a reason for hiding this comment

hidmic commented Jul 6, 2020

Arnatious commented Jul 6, 2020

dirk-thomas commented Jul 6, 2020

hidmic commented Jul 6, 2020 • edited Loading

Arnatious commented Jul 6, 2020

dirk-thomas commented Jul 6, 2020

Arnatious commented Jul 6, 2020

dirk-thomas commented Jul 6, 2020

hidmic commented Jul 14, 2020

emersonknapp commented Jul 14, 2020

emersonknapp commented Jul 14, 2020

kyrofa commented Jul 14, 2020 • edited Loading

Arnatious commented Jul 14, 2020

emersonknapp commented Jul 14, 2020 • edited Loading

Arnatious commented Jul 14, 2020

Arnatious commented May 12, 2020 •

edited

Loading

Arnatious commented May 14, 2020 •

edited

Loading

kyrofa May 15, 2020 •

edited

Loading

hidmic Jun 3, 2020 •

edited

Loading

hidmic Jun 3, 2020 •

edited

Loading

hidmic left a comment •

edited

Loading

hidmic commented Jul 6, 2020 •

edited

Loading

kyrofa commented Jul 14, 2020 •

edited

Loading

emersonknapp commented Jul 14, 2020 •

edited

Loading