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

added integration with aiohttp #18

Merged
merged 9 commits into from Jul 6, 2020

Conversation

amenezes
Copy link
Contributor

@amenezes amenezes commented Jul 1, 2020

Added initial integration with aiohttp #17.

@MatanRubin would you rather I use the notation below to fix the pylint warning or is there any other?

# current code
async def get_info(request: web.Request) -> web.Response:
            return web.json_response(self._to_dict(pyctuator_impl.app_info))

# fix W0613: Unused argument 'request' (unused-argument)
async def get_info(_: web.Request) -> web.Response:
            return web.json_response(self._to_dict(pyctuator_impl.app_info))

@michaelyaakoby
Copy link
Member

Hey Alexander, that was fast. Awesome.

While I didn't yet review this change, looking at the failed checks, I think many of them because the aiohttp extra module needs to be installed before pylint/mypy.
To do so, add --extras aiohttp to run: poetry install --extras flask --extras fastapi --extras db --extras redis in .github\workflows\python_package_build.yml.

If aiohttp doesn't publish stub, mypy will continue complaining, in this case you can exclude it in mypy.ini.

Also, pylint is complaining that Unused argument 'request' (unused-argument), guess you should tell it to ignore, add unused-argument to # pylint: disable=.

Thanks

@amenezes
Copy link
Contributor Author

amenezes commented Jul 2, 2020

Thank you for your help @michaelyaakoby. The commented steps solved the issue relative to CI.

The code base of the project is cohesing and I would really like to use this feature in an application in my work.

Copy link
Member

@michaelyaakoby michaelyaakoby left a comment

Choose a reason for hiding this comment

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

Hi Alexander,
We are very excited to get this in - its really awesome.

One thing I'm missing is adding aiohttp to the tests/test_pyctuator_e2e.py.
I've cloned your branch and have added this myself.
Not sure what's the preferred way of sharing this with you so you can add to your branch.
Shoud I create a PR? or send you a patch?

@michaelyaakoby
Copy link
Member

Hi @amenezes , I've created a pull-request to your repo adding aiohttp to E2E test.
Please review amenezes#1
Thanks.

Extend E2E to ru all tests when pyctuator is using aiohttp server
@amenezes
Copy link
Contributor Author

amenezes commented Jul 3, 2020

Sorry for delay @michaelyaakoby I made merge with your PR but now some error is happening with the aiohttp tests.

@michaelyaakoby
Copy link
Member

@amenezes, the failures seem like issues in pyctuator/impl/aiohttp_pyctuator.py, will be great if you can try and fix this (run tests.test_pyctuator_e2e.py specifying 'aiohttp' keyword).
I'll be able to look at this earlier next week.
Thanks

michael.yak and others added 2 commits July 4, 2020 02:02
@michaelyaakoby michaelyaakoby changed the base branch from master to amenezes/aiohttp July 6, 2020 20:21
@michaelyaakoby
Copy link
Member

Hi @amenezes I'm merging this to a branch where I'll add the E2E fix (which I sent you earlier this week) and we should release shortly.
Thanks
Michael

@michaelyaakoby michaelyaakoby merged commit 3b99b1c into SolarEdgeTech:amenezes/aiohttp Jul 6, 2020
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