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

Add path web app run #6842

Closed
wants to merge 9 commits into from
Closed

Add path web app run #6842

wants to merge 9 commits into from

Conversation

SamirPS
Copy link
Contributor

@SamirPS SamirPS commented Jul 23, 2022

What do these changes do?

I modify the web.py file and add Union[str,pathlib.Path] for path

Are there changes in behavior for the user?

For the option of path, we can have str or pathlib.path

Related issue number

#6839

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@SamirPS SamirPS requested a review from asvetlov as a code owner July 23, 2022 00:47
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 23, 2022
@Dreamsorcerer
Copy link
Member

@SamirPS SamirPS requested a review from webknjaz as a code owner July 23, 2022 12:09
@SamirPS
Copy link
Contributor Author

SamirPS commented Jul 23, 2022

Need to see a test covering this. Could probably just remove str() from these 2: https://github.com/aio-libs/aiohttp/blob/master/tests/test_run_app.py#L521 https://github.com/aio-libs/aiohttp/blob/master/tests/test_run_app.py#L535

Okay i remove str() for this two test and all test in this file passed

aiohttp/web.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

Also, test failing. Looks like there's another place that needs a Union[] annotation.

@SamirPS
Copy link
Contributor Author

SamirPS commented Jul 23, 2022

@Dreamsorcerer For me all test passed or skipped because i working on ubuntu (WSL)

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 23, 2022

This pull request introduces 1 alert when merging 6391e9e into 934db9a - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

@Dreamsorcerer
Copy link
Member

We run a lot of tests in CI, so check the output. You should be able to reproduce the mypy errors on your machine, by simply running mypy.

@SamirPS SamirPS closed this Jul 23, 2022
@SamirPS SamirPS deleted the SamirPS-add-path-web-app-run branch July 23, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants