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

Additional Pep8speaks updates #856

Merged
merged 8 commits into from
May 22, 2024

Conversation

BradleySappington
Copy link
Collaborator

Found a few missed items from the no-formatting transition

@BradleySappington BradleySappington self-assigned this May 21, 2024
@pep8speaks
Copy link

pep8speaks commented May 21, 2024

Hello @BradleySappington, Thank you for updating !

There are no PEP8 issues in this Pull Request.

Comment last updated at 2024-05-22 20:20:33 UTC

@BradleySappington BradleySappington changed the title DRAFT: Additional Pep8speaks updates Additional Pep8speaks updates May 21, 2024
Copy link
Collaborator

@mperrin mperrin left a comment

Choose a reason for hiding this comment

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

Hi @BradleySappington thanks for these edits. Most looks fine but I have a couple concerns, particularly about line breaks in strings.

I think using a backslash to break a string across two lines doesn't work in the way you're intending it to here: the whitespace at the start of the second line isn't trimmed out automatically. I tested several of those edits, and the proposed edits do result in extra unintended whitespace in the output strings, so I think those will need to be revised. There are quite a few instances of this, as marked below (I may not have caught them all).

This is a small thing but I don't think we want to accidentally introduce formatting errors in output messages in the course of cleaning up formatting errors of the code 😆

Copy link
Collaborator Author

@BradleySappington BradleySappington left a comment

Choose a reason for hiding this comment

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

Thanks again for pointing out the formatting mistake!

@BradleySappington BradleySappington changed the title Additional Pep8speaks updates WIP: Additional Pep8speaks updates May 22, 2024
@BradleySappington BradleySappington changed the title WIP: Additional Pep8speaks updates Additional Pep8speaks updates May 22, 2024
@BradleySappington
Copy link
Collaborator Author

@mperrin - updates complete

Copy link
Collaborator

@mperrin mperrin 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. Thanks @BradleySappington

@mperrin mperrin merged commit dfd4528 into spacetelescope:develop May 22, 2024
7 checks passed
@BradleySappington BradleySappington deleted the pep8speaks branch November 22, 2024 21:14
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.

3 participants