Skip to content

More Flake8 cleanup#229

Merged
aaronayres35 merged 19 commits into
masterfrom
flake8-round2
Nov 23, 2020
Merged

More Flake8 cleanup#229
aaronayres35 merged 19 commits into
masterfrom
flake8-round2

Conversation

@aaronayres35
Copy link
Copy Markdown
Contributor

part of #145
With this PR the codebase is very nearly fully flake8 clean. There is the possibility some of the remaining non-flake8 clean code will be killed as well.

Checklist
- [ ] Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

Comment thread docs/source/conf.py Outdated
# is relative to the documentation root, use os.path.abspath to make it
# absolute, like shown here.
#sys.path.append(os.path.abspath('some/directory'))
# sys.path.append(os.path.abspath('some/directory'))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I should probably just delete all these commented out code blocks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I'm not sure, some of it isn't really dead code, but rather unused options. I removed all of it in the latest commit, but if these should be kept I can simply revert that commit

@@ -17,11 +17,11 @@ def __init__(self, firstname, lastname):
class Foo:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TBH these integration tests could probably just be deleted. I don't believe they ever get run/can be run

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There might be values in converting them to test cases so that they can be discovered and run? In any case, that would be a separate issue. Agreed that tests don't get automatically run typically get forgotten and end up bring broken long before anyone realizes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. I will leave that for a separate issue/PR

Comment thread apptools/scripting/recorder.py Outdated
Comment on lines -407 to -410
if six.PY3:
file.write(self.get_code())
else:
file.write(six.text_type(self.get_code(), encoding="utf-8"))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change is not a flake8 cleaning, it is just related to dropping pyhton 2 support #120
I added it because the later case below with six.PY2 had a flake8 error, so I removed it, and it felt appropriate to remove them together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This sparked my opening of #230 which makes the same change, but goes further to replace all uses of six throughout the code base.

Copy link
Copy Markdown
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronayres35 aaronayres35 merged commit 688b14c into master Nov 23, 2020
@aaronayres35 aaronayres35 deleted the flake8-round2 branch November 23, 2020 14:18
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