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

Delete requirements.txt #1500

Closed
wants to merge 1 commit into from
Closed

Conversation

GrayHatter
Copy link

In after upgrading to python 3.7 and istalling with pip install https://github.com/Rapptz/discord.py/archive/async.zip, discord.py wouldn't play nicely with EITHER lib at these versions. When I told pip to ignore dependencies, discord.py started working again.

I'd be happy to fix up this commit with the actual required dependencies if you need them. But no matter what. these values are invalid for the async branch.

@Harmon758
Copy link
Contributor

Harmon758 commented Aug 11, 2018

Removing the requirements is not the way to go about this.
Not only will users of other Python versions also be forced to separately install the dependencies, with the library entirely broken until they did so, but there won't be a clear indication of what dependencies are required.

You're right that currently, the async branch doesn't work on Python 3.7. However, this is because the current pinned versions of aiohttp and websockets don't support Python 3.7, not because discord.py doesn't work with these versions. This is also a known issue; see #1401.
The latest versions of both dependencies do work on Python 3.7, but are not guaranteed to work properly with the async branch. Hence, they are pinned to the known working versions, and Python 3.7 support has not been added to the async branch.

@ghost
Copy link

ghost commented Aug 11, 2018

If 3.7 is a requirement you can always switch over to the rewrite branch

@GrayHatter
Copy link
Author

I was told not to use the rewrite branch yet before I was banned from the discord server. It the rewrite branch now safe to use?

Is there a test suite, or a standard test system that I could do to verify the async branch works with up to date dependencies?

Python 3.7 is now the primary version of python, (ignoring that python 2 still exists for some reason). But discord.py doesn't work with it at all (because pinned libs don't). So while I agree that this isn't the best solution, (given I already agreed to fix up the commit as needed). It seems to me, the options are

  1. do nothing: and discord.py is broken for new or recently updated installs.
  2. merge this as is: and it might work, it's working for me
  3. tell me how you'd like the dependencies to be updated/tested?

Unless I'm missing something?

@ghost
Copy link

ghost commented Aug 11, 2018

Rewrite has been safe to use for a long while. It implements everything async does and is fine for production.

@mental32
Copy link
Contributor

Rewrite is perfectly fine to use it’s the most up to date branch since async is no longer getting new features and, barring voice, works fine on 3.7

Your only issue migrating is porting the code because rewrite is not backwards compatible with async at all. It’s documentation even includes a migration guide

@Harmon758
Copy link
Contributor

Is there a test suite, or a standard test system that I could do to verify the async branch works with up to date dependencies?

In short, no. Unit tests for Discord bots/libraries aren't really feasible.

Python 3.7 is now the primary version of python, (ignoring that python 2 still exists for some reason).

Not sure what you mean by primary, but Python 3.4-3.6 are still fully supported, and Python 2.7 EOL isn't until 2020, not that that's relevant.

do nothing: and discord.py is broken for new or recently updated installs.

New installations of Python 3.4-3.6 are still perfectly possible, and updating between major versions of Python without a new installation isn't possible.

merge this as is: and it might work, it's working for me

This doesn't really make sense. Like I said, the library would not work at all, on any version of Python, for anyone installing it without aiohttp and websockets previously installed. They would be installing it without either dependency because the requirements would be missing. Not only that, but I expect the setup itself would error, since it expects a requirements.txt file, and installing the library at all would probably not be possible. There is no possibility of it "might work"ing. It would definitively not work.
There is no way that discord.py works for you without both aiohttp and websockets, because they are required dependencies; there is no way that you installed discord.py without a requirements.txt file and have it working without somehow separately installing aiohttp and websockets; and there is likely no way that you even installed discord.py without a requirements.txt file at all.

@GrayHatter
Copy link
Author

In short, no. Unit tests for Discord bots/libraries aren't really feasible.

why's that?

there is no way that you installed discord.py without a requirements.txt file

My apologies, I was under the impression that pip stored dependencies independently from requirements.txt and when I didn't see aiohttp nor websockets in setup.py, but you're pulling I've updated the commit to just remove the upper bounds.

New installations of Python 3.4-3.6 are still perfectly possible, and updating between major versions of Python without a new installation isn't possible.

That dichotomy doesn't seem to be relevant. I updated my server, and discord.py stopped working because it's requirements. If I spin up a new server, I'd have to install python 3.6 out of band, instead of just unconstraining the requirements. If discord.py doesn't work with the most up to date versions of it's dependent libs, that's discord.py's issue... which I'm also happy to fix in an additional pull if there's any new bugs reported from this patch. Because as I said, I'm not having any issues with them at their current versions.

All that said, if the rewrite branch is nearly equivalent to the async branch, what's blocking it from replacing async? Maybe it's better easier to fix that instead and marking the async branch as legacy?

@ghost
Copy link

ghost commented Aug 11, 2018

There's not much point arguing as async doesn't get any updates. There's not much stopping rewrite from becoming main. It surpasses async on completeness and implementation. We're just waiting on voice receive pretty much. Lots of us are hoping the switch comes soon.

@ClementJ18
Copy link
Contributor

Testing wise: Because creating mocks of the discord models without emulating the api itself is complex.

In after upgrading to python 3.7 and istalling with `pip install https://github.com/Rapptz/discord.py/archive/async.zip`, discord.py wouldn't play nicely with EITHER lib at these versions. When I told pip to ignore dependencies, discord.py started working again.

I'd be happy to fix up this commit with the actual required dependencies if you need them. But no matter what. these values are invalid for the async branch.
@GrayHatter
Copy link
Author

I did fix the pull to just remove the upper limit of the dependencies. However...

If async is gonna be replaced soon, there's no need to find out what versions async is gonna need.
I think this pull is safe close this then. I still think merging this is better than nothing, as it wont hurt existing installs, but new installs on python 3.7 will start to work.

I'm going to retract my offer to verify the exact versions of libs needed, but I'm happy to replace it with adding audio support to the rewrite branch. Only the work/specific tasks still needing work on rewrite isn't listed anywhere obvious.

@Tobotimus
Copy link
Contributor

@GrayHatter If you want to run async, you need to run it with the dependency constraints listed. The author put them there for a reason, and they're not going to change for async. In case you are not aware, it's entirely possible to have multiple versions of python installed on the same system - virtualenvs will help you out here. Not trying to be rude but if you want to save Rapptz some time, close the PR

@GrayHatter
Copy link
Author

GrayHatter commented Aug 14, 2018 via email

@Werseter
Copy link

Have you tried doing so on a fresh machine? :)

Seriously, drop it. We have pointed out your mistakes already. You have not fixed anything, you are basing your findings on a false positive.

@GrayHatter
Copy link
Author

GrayHatter commented Aug 15, 2018 via email

@Rapptz Rapptz added the invalid This is not right. label Aug 23, 2018
@Rapptz Rapptz closed this Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This is not right.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants