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

setup.py: fix requirements #288

Merged
merged 14 commits into from
Nov 29, 2018
Merged

setup.py: fix requirements #288

merged 14 commits into from
Nov 29, 2018

Conversation

blueyed
Copy link
Member

@blueyed blueyed commented Jul 15, 2018

Fixes #287

Please review carefully.

Based on pytest's setup.py, which supports different marker levels, but I hope we do not need that: https://github.com/pytest-dev/pytest/blob/master/setup.py.

@blueyed
Copy link
Member Author

blueyed commented Jul 15, 2018

I think requirements.txt could also be moved into setup.py - but left it for now.

@blueyed
Copy link
Member Author

blueyed commented Jul 15, 2018

I think this warrants a 0.3.x release already, since people are having problems with it everywhere already.

blueyed added 2 commits July 15, 2018 03:09
- use tox-travis
- upload to codecov
- test py35, py36
@codecov-io
Copy link

codecov-io commented Jul 15, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@93e8753). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #288   +/-   ##
=========================================
  Coverage          ?   70.77%           
=========================================
  Files             ?      133           
  Lines             ?     7737           
  Branches          ?     1252           
=========================================
  Hits              ?     5476           
  Misses            ?     1932           
  Partials          ?      329

@Kuniwak
Copy link
Member

Kuniwak commented Jul 15, 2018

@blueyed I appreciate to your contributing. I have not tested for reproducing but I have a question about why past tests on Python 2.7 were not failed by the cause. In my first look, I think your changes actually do the same thing like the previous code. Would you explain how work your fix?

@blueyed
Copy link
Member Author

blueyed commented Jul 15, 2018

why past tests on Python 2.7 were not failed by the cause

have not checked myself

The main problem is that the conditionals only get executed when running setup.py, which is not the case for when a wheel (.whl) is used: there it depends on where the wheel was built.

@blueyed
Copy link
Member Author

blueyed commented Jul 15, 2018

I think this is good from my side.
What do you think about moving requirements.txt also into setup.py?

.travis.yml Outdated

install:
- pip install tox
- if test "$TOXENV" = "coverage"; then pip install coveralls; fi
- pip install tox tox-travis pytest-cov
Copy link
Member Author

Choose a reason for hiding this comment

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

pytest-cov would not be necessary here. But could maybe get removed from testing requirements?!

@@ -1,17 +1,9 @@
[tox]
envlist = py27, py34, coverage
envlist = py27, py36
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 list gets used when using just ´tox` locally, so I have not listed all envs there.

@Kuniwak
Copy link
Member

Kuniwak commented Jul 18, 2018

What do you think about moving requirements.txt also into setup.py?

@blueyed Sounds good. And keeping the requirements.txt file (it will become to be empty) may be good to old users. Because previous installation instructions are using requirements.txt.

Copy link
Member

@Kuniwak Kuniwak left a comment

Choose a reason for hiding this comment

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

Looks good to me. I appreciate your contribution!

@Kuniwak
Copy link
Member

Kuniwak commented Jul 18, 2018

@blueyed I'm planning that releasing this fix at both 0.3.20 and 0.4a2. How do you feel it?

@blueyed
Copy link
Member Author

blueyed commented Jul 19, 2018

@Kuniwak
Sounds good to me!

Would be good to ensure that the built wheels are working as expected though - when being built locally and installed in different Python versions.

blueyed added a commit to JelteF/vim-python-pep8-indent that referenced this pull request Jul 26, 2018
It is always failing, not worth fixing for now.

Ref: Vimjas/vint#288
Ref: Vimjas/vint#287
@blueyed

This comment has been minimized.

@blueyed

This comment has been minimized.

blueyed added a commit to blueyed/vint that referenced this pull request Sep 8, 2018
@blueyed
Copy link
Member Author

blueyed commented Sep 8, 2018

Pushed a few commits.
I think we should merge this finally soon.

@lambdalisue
Copy link
Contributor

Updates?

@blueyed
Copy link
Member Author

blueyed commented Nov 21, 2018

No, it seems like @Kuniwak went missing.. :/

I could merge this myself, but would like to get more eyes on this first.

@blueyed blueyed merged commit 259395c into Vimjas:master Nov 29, 2018
@blueyed blueyed deleted the fix-reqs branch November 29, 2018 14:51
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.

4 participants