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.

Sign up for GitHub

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

Jump to bottom

Fix unit test and lint issues #216

Closed
wants to merge 1 commit into from
Closed

Fix unit test and lint issues #216

wants to merge 1 commit into from

Conversation

greyli
Copy link
Member

@greyli greyli commented Nov 15, 2023 •
edited by jeffwidman

@greyli greyli mentioned this pull request Nov 15, 2023
Merged
@jeffwidman
Copy link
Member

jeffwidman commented Nov 15, 2023 •
edited

Thanks!

Can you actually break this apart into 3 different PR's?
They are unrelated changes, and I had questions on one or two of these but don't want to block the others.

In particular, we need to land the before_first_request to get CI working, and from there we can start merging other PRs.

jeffwidman
jeffwidman reviewed Nov 15, 2023
View reviewed changes
test/test_toolbar.py
@@ -7,7 +7,6 @@

def load_app(name):
app = __import__(name).app
app.config['TESTING'] = True
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be removed, but best to discuss/research in a separate PR that is specific to this TESTING/setting DEBUG...

jeffwidman
jeffwidman reviewed Nov 15, 2023
View reviewed changes
tox.ini
@@ -13,7 +13,7 @@ commands =
deps =
pycodestyle
commands =
pycodestyle flask_debugtoolbar test
pycodestyle src/flask_debugtoolbar test --ignore=E731,W503,W504
Copy link
Member

Choose a reason for hiding this comment

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

this would be nicer to annotate with the reasoning for adding these ignores in a separate PR. No need for a code comment on them, but at least a PR comment woudl be nice.

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to open a PR for this if you think it's useful!

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #219

macnewbold
macnewbold reviewed Nov 16, 2023
View reviewed changes
Copy link
Contributor

@macnewbold macnewbold left a comment

Choose a reason for hiding this comment

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

I reviewed this, and don't have any concerns beyond the good feedback from @jeffwidman

@greyli
Copy link
Member Author

greyli commented Nov 16, 2023

Good point. I will create the separate PRs and let's discuss the details later.

@greyli greyli closed this Nov 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeffwidman jeffwidman jeffwidman left review comments

@macnewbold macnewbold macnewbold left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@greyli @jeffwidman @macnewbold