Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added undescore to ignore new pydocstyle item #428

Merged
merged 5 commits into from
Jan 3, 2023

Conversation

Crola1702
Copy link
Contributor

Signed-off-by: Crola1702 cristobal.arroyo@ekumenlabs.com

Pydocstyle 6.2.0 introduced a new feature where get_files_to_check method yields 4 items instead of 3.

This PR adds and ignores that item, so the tests can run without crashing.

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
@Crola1702
Copy link
Contributor Author

Rolling CI is installing Pydocstyle 6.1.1:

00:01:00.043 pydocstyle: 6.1.1-1

@Crola1702
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Linux CI is upgrading to 6.2.1

Collecting pydocstyle
  Downloading pydocstyle-6.2.1-py3-none-any.whl (37 kB)
Collecting pyflakes
  Downloading pyflakes-3.0.1-py2.py3-none-any.whl (62 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 62.8/62.8 KB 11.4 MB/s eta 0:00:00
...
pydocstyle==6.2.1

@@ -187,7 +187,7 @@ def generate_pep257_report(paths, excludes, ignore, select, convention, add_igno
report = []

files_dict = {}
for filename, checked_codes, ignore_decorators in files_to_check:
for filename, checked_codes, ignore_decorators, _ in files_to_check:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this isn't going to work with older pydocstyle, which we still need to support for users who haven't upgraded the pydocstyle yet.

Instead, I think we'll need to first try to unpack 3 values, and if that fails with a ValueError, then try to unpack with 4 values. Alternatively, we could look up the version number of pydocstyle that is currently in use, and choose which one to do based on that (though I think that is a little more brittle).

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
@Crola1702 Crola1702 requested review from clalancette and removed request for mjeronimo January 3, 2023 16:35
Comment on lines 190 to 207
# Unpack 3 values for pydocstyle <= 6.1.1
try:
for filename, checked_codes, ignore_decorators in files_to_check:
if _filename_in_excludes(filename, excludes):
continue
files_dict[filename] = {
'select': checked_codes,
'ignore_decorators': ignore_decorators,
}
# Unpack 4 values since pydocstyle >= 6.2.0
except ValueError:
for filename, checked_codes, ignore_decorators, _ in files_to_check:
if _filename_in_excludes(filename, excludes):
continue
files_dict[filename] = {
'select': checked_codes,
'ignore_decorators': ignore_decorators,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely works, but duplicates the body of the loop.

I actually just learned about a new Python 3 syntax that lets you unpack an optional set of arguments at the end, *_. So I think we can handle returning 3 or 4 tuple items, without duplicating the body of the loop, with:

Suggested change
# Unpack 3 values for pydocstyle <= 6.1.1
try:
for filename, checked_codes, ignore_decorators in files_to_check:
if _filename_in_excludes(filename, excludes):
continue
files_dict[filename] = {
'select': checked_codes,
'ignore_decorators': ignore_decorators,
}
# Unpack 4 values since pydocstyle >= 6.2.0
except ValueError:
for filename, checked_codes, ignore_decorators, _ in files_to_check:
if _filename_in_excludes(filename, excludes):
continue
files_dict[filename] = {
'select': checked_codes,
'ignore_decorators': ignore_decorators,
}
# Unpack 3 values for pydocstyle <= 6.1.1, and 4 values for pydocstyle >= 6.2.0.
for filename, checked_codes, ignore_decorators, *_ in files_to_check:
if _filename_in_excludes(filename, excludes):
continue
files_dict[filename] = {
'select': checked_codes,
'ignore_decorators': ignore_decorators,
}

Can you give this a try and see if it still solves the problem?

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with green CI!

Please make sure to run CI up to some package that uses ament_pep257, as well as on RHEL (to make sure it still works there).

@Crola1702
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Rhel Build Status

@clalancette
Copy link
Contributor

This is all green, so merging this in.

@clalancette clalancette merged commit ae431ed into ament:rolling Jan 3, 2023
@clalancette
Copy link
Contributor

@Mergifyio backport humble

mergify bot pushed a commit that referenced this pull request Jan 6, 2023
* Added underscore to ignore new item

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
(cherry picked from commit ae431ed)
@mergify
Copy link

mergify bot commented Jan 6, 2023

backport humble

✅ Backports have been created

audrow pushed a commit that referenced this pull request Jan 12, 2023
* Added underscore to ignore new item

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
(cherry picked from commit ae431ed)
Signed-off-by: Audrow Nash <audrow@openrobotics.org>
audrow pushed a commit that referenced this pull request Jan 12, 2023
* Added underscore to ignore new item

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
(cherry picked from commit ae431ed)
Signed-off-by: Audrow Nash <audrow@openrobotics.org>

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
Signed-off-by: Audrow Nash <audrow@openrobotics.org>
Co-authored-by: Cristóbal Arroyo <69475004+Crola1702@users.noreply.github.com>
@Crola1702
Copy link
Contributor Author

@mergify backport foxy

@mergify
Copy link

mergify bot commented Jan 12, 2023

backport foxy

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • sender-permission>=write

@clalancette
Copy link
Contributor

@Mergifyio backport foxy

mergify bot pushed a commit that referenced this pull request Jan 12, 2023
* Added underscore to ignore new item

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
(cherry picked from commit ae431ed)

# Conflicts:
#	ament_pep257/ament_pep257/main.py
@mergify
Copy link

mergify bot commented Jan 12, 2023

backport foxy

✅ Backports have been created

clalancette pushed a commit that referenced this pull request Jan 19, 2023
* Added underscore to ignore new pydocstyle item (#428)

* Added underscore to ignore new item

Signed-off-by: Crola1702 <cristobal.arroyo@ekumenlabs.com>
(cherry picked from commit ae431ed)
@ijnek
Copy link

ijnek commented Mar 3, 2023

Could we get a release in foxy so we can get this change please? I believe everyone's foxy CIs for ros2 python packages are failing without this change right now. Thanks!

@Crola1702
Copy link
Contributor Author

Could we please get a release in foxy so we can get this change please?

FYI: @jacobperron

@clalancette
Copy link
Contributor

I'll comment over on the backport PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants