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

Lint CONTRIBUTORS.txt alphabetical order #3328

Merged
merged 2 commits into from
Oct 8, 2018

Conversation

pollydrag
Copy link
Contributor

@pollydrag pollydrag commented Oct 6, 2018

What do these changes do?

Lint CONTRIBUTORS.txt alphabetical order.
In CI and "make flake"

Are there changes in behavior for the user?

Only lint, does not affect behavior.

Related issue number

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."

@pollydrag
Copy link
Contributor Author

Is a change-note required for this?

@codecov-io
Copy link

codecov-io commented Oct 6, 2018

Codecov Report

Merging #3328 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3328   +/-   ##
=======================================
  Coverage   98.01%   98.01%           
=======================================
  Files          43       43           
  Lines        8025     8025           
  Branches     1356     1356           
=======================================
  Hits         7866     7866           
  Misses         66       66           
  Partials       93       93

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efd6073...72d89b6. Read the comment docs.

.travis.yml Outdated
@@ -39,6 +39,7 @@ _helpers:
<<: *_reset_steps
install:
- *upgrade_python_toolset
- pip install -U -r requirements/cython.txt
Copy link
Member

Choose a reason for hiding this comment

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

This is totally unrelated and not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we get an error without this line.

$ python --version
Python 3.6.3
$ pip --version
pip 9.0.1 from /home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages (python 3.6)
Skipping the before_install step, as specified in the configuration.
install.1
3.63s$ pip install --upgrade pip wheel setuptools
Collecting pip
  Downloading https://files.pythonhosted.org/packages/c2/d7/90f34cb0d83a6c5631cf71dfe64cc1054598c843a92b400e55675cc2ac37/pip-18.1-py2.py3-none-any.whl (1.3MB)
    100% |████████████████████████████████| 1.3MB 1.2MB/s 
Collecting wheel
  Downloading https://files.pythonhosted.org/packages/fc/e9/05316a1eec70c2bfc1c823a259546475bd7636ba6d27ec80575da523bc34/wheel-0.32.1-py2.py3-none-any.whl
Collecting setuptools
  Downloading https://files.pythonhosted.org/packages/96/06/c8ee69628191285ddddffb277bd5abdf769166e7a14b867c2a172f0175b1/setuptools-40.4.3-py2.py3-none-any.whl (569kB)
    100% |████████████████████████████████| 573kB 2.6MB/s 
Installing collected packages: pip, wheel, setuptools
  Found existing installation: pip 9.0.1
    Uninstalling pip-9.0.1:
      Successfully uninstalled pip-9.0.1
  Found existing installation: wheel 0.30.0
    Uninstalling wheel-0.30.0:
      Successfully uninstalled wheel-0.30.0
  Found existing installation: setuptools 38.2.4
    Uninstalling setuptools-38.2.4:
      Successfully uninstalled setuptools-38.2.4
Successfully installed pip-18.1 setuptools-40.4.3 wheel-0.32.1
2.47s$ pip install -U -r requirements/ci.txt
Obtaining file:///home/travis/build/pollydrag/aiohttp (from -r requirements/ci.txt (line 6))
  Installing build dependencies ... done
    Complete output from command python setup.py egg_info:
    Install cython when building from git clone
    Hint:
      pip install cython

https://travis-ci.org/pollydrag/aiohttp/jobs/437842307

It because lint_base.install try to install requirements/ci.txt but cython is not installed.
It was not noticeable, because all lint steps has own install.
Maybe remove install section from lint_base?

Copy link
Member

Choose a reason for hiding this comment

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

Just override install with install: skip in job you're adding. I don't want to influence that part now. I'll take care of it later.

.travis.yml Outdated
@@ -180,6 +181,12 @@ jobs:
script:
- python setup.py check --metadata --restructuredtext --strict --verbose

- <<: *_lint_base
env:
- TARGET="'CONTRIBUTORS.txt sorting check'"
Copy link
Member

Choose a reason for hiding this comment

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

Now instead of this env var I encourage you to use:

name: CONTRIBUTORS.txt sorting check

Copy link
Member

Choose a reason for hiding this comment

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

(however I'm not sure whether we're okay with a separate job overhead)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 It look great:

image

.travis.yml Show resolved Hide resolved
.travis.yml Outdated
@@ -37,9 +37,6 @@ _helpers:
stage: &doc_stage_name docs, linting and pre-test checks
<<: *_mainstream_python_base
<<: *_reset_steps
install:
Copy link
Member

Choose a reason for hiding this comment

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

Hm.. it's odd. can you rebase on top of master?

Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't delete this.

Copy link
Member

Choose a reason for hiding this comment

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

Any reason you wanted to delete it?

.travis.yml Outdated
@@ -145,14 +142,12 @@ jobs:
sudo: required

- <<: *_doc_base
env:
- TARGET=docs
name: docs
Copy link
Member

Choose a reason for hiding this comment

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

These changes are not related. Only edit what's related to your PR.

.travis.yml Show resolved Hide resolved
@asvetlov
Copy link
Member

asvetlov commented Oct 8, 2018

@webknjaz please merge when ready

@webknjaz
Copy link
Member

webknjaz commented Oct 8, 2018

Sure

@webknjaz
Copy link
Member

webknjaz commented Oct 8, 2018

@pollydrag thanks!

@webknjaz webknjaz merged commit 929e197 into aio-libs:master Oct 8, 2018
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants