Refactors code for currently supported Python 3.6 to 3.9

[funkyfuture]

i took the time to remove the Python 2 specific code. also i think it's easier to maintain only currently supported Python versions. there's still a lot of opportunities to improve the code and i hope to have the time to contribute more.

[coveralls]

Coverage decreased (-0.04%) to 84.81% when pulling 94c8948 on funkyfuture:python3 into 7428218 on joalla:master.

[alifhughes]

Hey @funkyfuture, thank you for the pull request! Sorry for the delay in the response, I thought I had set up notifications on this repo but it seems that this one had slipped through the net. I'm going to look into this over the next 24hours. First thing I have noticed however was the failing CI (that isn't being reported correctly.. which I'll also look into). See here, seems to be running the nosetests --with-coverage --cover-package=discogs_client with the new versions of python that you've enabled. I can also take a look at these too.

[JKatzwinkel]

Seems like coverage version as specified in requirements.txt is just too old. Tests succeed with e.g. coverage>=5.0.3 or coverage>=5.3 and Python 3.7, 3.8 and 3.9, but not with Python 3.6.

Maybe coverage could be disabled for 3.6 with something like:

script: |
  if [ $(python -c "import sys; print(sys.version_info.minor)") -lt 7 ];
    nosetests
  else
    nosetests --with-coverage --cover-package=discogs_client
  fi

Also, maybe nose should be listed in requirements.txt just to be safe?

[JKatzwinkel]

Suggested change

	install:
	- pip install -r requirements.txt
	install: |
	pip install -r requirements.txt
	pip install --upgrade coverage==5.0.3

[alifhughes]

I think we could just specify coverage==5.0.3 in requirements.txt along with nose as you've suggested above. Seems strange to have specific version specified here and a different one in the requirements.txt

[JKatzwinkel]

yes that was my intention also, I just felt that I had to leave it somewhere and I couldn't suggest it in requirements.txt directly without creating a fork.

[alifhughes]

Yea no worries! I only realised that after putting that comment to as I was attempting to write it as a suggestion on here

[funkyfuture]

thank you for looking into this. i'm just not sure whether it's worth the hassle, as me thinks switching to pytest is imperative and should be one of the next major code updates.

@alifhughes, i have no strong opinion on this. if you want to integrate @JKatzwinkel's suggestions, i'm fine with this. (assuming your review interface enables you to do so as mine does.)

[JKatzwinkel]

I don't think that switching to pytest would make coverage suddenly work with python 3.6.0 if I'm tbh.

[alifhughes]

Cheers for looking into the Travis issue @JKatzwinkel, to be honest, my intention was never to have coverage get in the way of development, merely a measure that we can keep an eye on, so @funkyfuture I am happy for you to include @JKatzwinkel 's suggestion here, which will disable coverage on python 3.6. As I mentioned here I think it would be good to bump the coverage version in the requirements.txt and include nose there too as @JKatzwinkel has suggested 👍

I am in agreement with @JKatzwinkel, that although moving to pytest would be a great improvement overall, it wouldn't fix the coverage issue on 3.6.

Am going to look into the actual changes now

[alifhughes]

As we've now removed the need for six in the project here. I think we can remove it from the requirements.txt too

[alifhughes]

Generally code changes LGTM 👍

I've added one minor comment and also if you could apply changes to get Travis ignoring python 3.6 as well, then I'm happy to merge 🎉

[JKatzwinkel]

wait what do you mean by Travis ignoring python 3.6!?

[alifhughes]

wait what do you mean by Travis ignoring python 3.6!?

Sorry I should have been more specific! What I meant was: Travis to ignore/not do coverage with python 3.6 😄

[JOJ0]

Thanks to all of you for these essential improvements of discogs_client! Appreciated! :-)

[alifhughes]

@funkyfuture I hope you don't mind but I took the liberty to update the requirements.txt here as @JOJ0 updated .travis.yml. These were the last things needed to do before we can merge 👍

[alifhughes]

Travis is now playing up, however I am going to tackle this separately, I saw that the builds were green so I am going to approve and merge. Thanks for the discussion and suggestions everyone!