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

1795 rename async module #1796

Merged
merged 8 commits into from Jun 17, 2018

Conversation

Projects
None yet
5 participants
@diegoholiveira
Contributor

diegoholiveira commented May 26, 2018

Closes #1795

@benoitc

This comment has been minimized.

Owner

benoitc commented May 26, 2018

renaming it _async would mean that module is private where some can import its objects to create other async workers. I would rather rename it as async_base or base_async whatever.

@benoitc

see comment above

@diegoholiveira

This comment has been minimized.

Contributor

diegoholiveira commented May 26, 2018

changes requested done ;)

@benoitc

Thanks! looks good for me, just wondering if the change should be major. Thoughts ? cc @berkerpeksag @tilgovi

@benoitc benoitc requested a review from berkerpeksag May 27, 2018

@benoitc

This comment has been minimized.

Owner

benoitc commented Jun 1, 2018

@tilgovi @berkerpeksag thoughts? I am thinking we should create a pre-20 branch that include that change and the removal of python 2. Thoughts?

@berkerpeksag

Thanks! looks good for me, just wondering if the change should be major.

I don't think this is a major change since the chance of having worker subclasses is low and we are not the only project to make this change.

I'd vote +1 for adding this to the next 19.x release.

@@ -1,3 +1,3 @@
coverage>=4.0,<4.4 # TODO: https://github.com/benoitc/gunicorn/issues/1548
pytest==3.0.5
pytest-cov==2.4.0
pytest

This comment has been minimized.

@berkerpeksag

berkerpeksag Jun 3, 2018

Collaborator

Please leave unrelated changes to another PR.

19.9.0 / 2018/05/26
===================
- the internal module `gunicorn.workers.async` was renamed to `gunicorn.workers.base_async`

This comment has been minimized.

@berkerpeksag

berkerpeksag Jun 3, 2018

Collaborator

the -> The

19.9.0 / 2018/05/26
===================
- the internal module `gunicorn.workers.async` was renamed to `gunicorn.workers.base_async`

This comment has been minimized.

@berkerpeksag

berkerpeksag Jun 3, 2018

Collaborator

Style nit: Please use double backtics.

@@ -2,6 +2,14 @@
Changelog
=========
19.9.0 / 2018/05/26

This comment has been minimized.

@berkerpeksag

berkerpeksag Jun 3, 2018

Collaborator

I would change 2018/05/26 with "not released".

.gitignore Outdated
@@ -15,3 +15,5 @@ examples/frameworks/pylonstest/pylonstest.egg-info/
MANIFEST
nohup.out
setuptools-*
.cache

This comment has been minimized.

@berkerpeksag

berkerpeksag Jun 3, 2018

Collaborator

.cache was renamed to .pytest_cache and we already added it to .gitignore.

.gitignore Outdated
@@ -15,3 +15,5 @@ examples/frameworks/pylonstest/pylonstest.egg-info/
MANIFEST
nohup.out
setuptools-*
.cache
.eggs

This comment has been minimized.

@berkerpeksag

berkerpeksag Jun 3, 2018

Collaborator

This looks like a good candidate for having a global .gitignore file.

@diegoholiveira

This comment has been minimized.

Contributor

diegoholiveira commented Jun 3, 2018

@berkerpeksag

Looks great, thank you!

@benoitc benoitc self-requested a review Jun 6, 2018

@@ -17,6 +17,6 @@
}
if sys.version_info >= (3, 3):
if sys.version_info >= (3, 4):
# gaiohttp worker can be used with Python 3.3+ only.

This comment has been minimized.

@mozillazg

mozillazg Jun 8, 2018

3.3+ -> 3.4+?

This comment has been minimized.

This comment has been minimized.

@mozillazg

mozillazg Jun 9, 2018

I mean the comment # gaiohttp worker can be used with Python 3.3+ only. should be updated to # gaiohttp worker can be used with Python 3.4+ only. to match your change.

@tilgovi tilgovi merged commit bd833e0 into benoitc:master Jun 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jun 17, 2018

Merged! Thank you all for the PR and for the reviews. I'll try to make a release by next weekend so that hopefully Gunicorn is ready when 3.7 is released.

@berkerpeksag

This comment has been minimized.

Collaborator

berkerpeksag commented Jun 18, 2018

Can we please use the "squash and merge" option when there are intermediate commits in a PR? Commits like 07dc716 and 66ec021 shouldn't be merged into the master branch.

We don't have feature branches and develop features the way the Linux kernel team does (long lived branches, no intermediate commits etc.), so I think we can even disable the "create a merge commit" option.

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jun 18, 2018

I agree that we should avoid having fixup commits like that. They're not useful.

I don't "squash and merge" because sometimes the commits matter. I'll try to be more careful, though, and review the commits.

Maybe we can agree on a review process. When you give a ✔️ a usually trust it, but maybe we should ask for rebase / squash by the authors before we approve. Small risk of making people frustrated, but lower risk of accidentally squashing useful history or merging useless history.

@benoitc

This comment has been minimized.

Owner

benoitc commented Jun 20, 2018

@berkerpeksag well we should probably have long lived branch. So we can break from now the code and work on next major release and start new iterations from it :) For example, I wouldn't have merged it before bumping our own version. That change will break anyone using a custom worker based on the async base. So that's a good call for such branches :)

@diegoholiveira diegoholiveira deleted the diegoholiveira:1795-rename-async-module branch Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment