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

Update CI scripts to match httpcore #1043

Merged
merged 7 commits into from
Sep 6, 2020
Merged

Update CI scripts to match httpcore #1043

merged 7 commits into from
Sep 6, 2020

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Sep 5, 2020

Apologies in advance: this is quite a lot of stuff in not many commits. It was a kind of "copy as much as possible verbatim from httpcore and then make it work" task 😬

Here's a rough summary of all that this changes:

  • Copy in all new scripts/<> files from httpcore, adjust any existing to match
  • Make Github Actions workflows the same as httpcore (these really should be identical now)
  • Add a setup.cfg file, mostly just copying from httpcore again
  • Add flake8 to requirements and make it pass (mostly line length issues, but a few other small things to fix)
  • Add packaging tools to requirements file since they aren't installed in scripts/publish anymore
  • Update gitignore to be like httpcore, was missing build/dist/site directories

@@ -12,6 +12,7 @@ ujson
autoflake
black==20.8b1
databases[sqlite]
flake8
Copy link
Member Author

Choose a reason for hiding this comment

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

Not doing flake8-bugbear or flake8-pie for now. They're not passing and this PR is big enough already.


[mypy-tests.*]
disallow_untyped_defs = False
# check_untyped_defs = True
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't pass yet. Not sure whether to remove or add a TODO comment or something?

Copy link
Member

Choose a reason for hiding this comment

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

Probably remove this tests.* section and log a ticket to "Add type checking to test suite"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I created #1045 (open issue number 100 😰). I think it makes sense to leave mypy-tests.* in here and have mypy run against the tests directory like we do so that it can still check (very basic) things and at least we don't regress.

@@ -520,20 +520,20 @@ async def lifespan(self, scope: Scope, receive: Receive, send: Send) -> None:
"""
first = True
app = scope.get("app")
message = await receive()
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused variable picked up by flake8.

@@ -130,7 +130,7 @@ async def app(request):
@app.route("/dashboard/decorated")
@async_inject_decorator(additional="payload")
@requires("authenticated")
async def decorated_sync(request, additional):
async def decorated_async(request, additional):
Copy link
Member Author

Choose a reason for hiding this comment

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

These two functions were redefinitions which flake8 picked up. I tried to give them better names.

tests/test_routing.py Outdated Show resolved Hide resolved
@JayH5 JayH5 requested a review from a team September 5, 2020 15:24
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Great! :) A few suggestions but approving in advance...

scripts/build Outdated Show resolved Hide resolved
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

2 participants