manrajgrover / halo

💫 Beautiful spinners for terminal, IPython and Jupyter
MIT License
2.86k stars 148 forks source link

Improvement: Don't fix the dependency versions #126

Closed krischer closed 4 years ago

krischer commented 5 years ago

Description of new feature, or changes

This PR changes the versions of all requirements to greater or equal the existing version number. It currently is very easy to break halo just by installing a newer package version and pkg_config will complain. This unnecessarily restricts the number of environments in which halo can be used.

I don't think there is any reason not to do this and one can probably assume that most packages will be backwards compatible.

If you'd be okay with it I'd propose to get rid of the requirements.txt (and its dev version) and instead just put them in setup.py directly. I can offer to implement this. There is a short discussion about the merits of each approach here:

https://pip.readthedocs.io/en/1.1/requirements.html

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 405


Totals Coverage Status
Change from base Build 398: -2.2%
Covered Lines: 297
Relevant Lines: 330

💛 - Coveralls
embray commented 5 years ago

Yes, this is costing me several hours of now having to sort out some dependency conflicts. I will probably have to vendor this package and a few of its dependencies until this can be resolved.

embray commented 5 years ago

To add, I was cranky about this last night so I apologize for that--this is a really lovely package overall which is why I was so determined to use it despite the dependency hell. Definitely happy to help if I can.

manrajgrover commented 5 years ago

@embray Thanks for supporting this PR. I'm not sure what issues you've faced exactly. Kindly share your experience which might help in this PR.

rgcr commented 5 years ago

@embray Thanks for supporting this PR. I'm not sure what issues you've faced exactly. Kindly share your experience which might help in this PR.

Multiple times I had to fix the dependencies manually because of this: ... halo has requirement colorama==0.3.9, but you'll have colorama 0.4.1 which is incompatible

IMHO it's a waste of time to fix this manually every time I have an error like that and this should not be happening.

manrajgrover commented 5 years ago

@rgcr It completely makes sense. However, having a dependency version as colorama>=0.3.9 forces it to upgrade for major version changes also which may break halo. I think we should avoid upgrading to them automatically. I feel we should check for that and make it more strict i.e. colorama>=0.3.9,<=1.0.0. Correct me if I'm missing something or overthinking this.

krischer commented 5 years ago

Logically I'd be on board with not allowing major version jumps. In practice though Python modules seem to really rarely break their major APIs. In many cases (especially with 0.x.x versions) projects just decide that they are now major enough to slap the 1.0.0 version on it.

What I'm trying to say is that I think it is easier to just set the minimum version (or maybe not set it at all if there is no reason). If there is indeed a breaking version change with one of the dependencies, a limitation can be added but I think this would happen less often than the other way around. Another advantage of not fixing the dependency versions is that other projects using halo can just fix the other dependencies themselves in case an error occurs (which is a lot quicker than submitting something upstream and waiting till it propagates).

Let me know if you disagree and I'll add the restriction on major version jumps.

I just rebased this on the latest master.

krischer commented 5 years ago

Can you also upgrade the patch version for halo to 0.0.26? Kindly make the necessary changes and let me know once done.

I updated it to 0.0.27 as it already was on 0.0.26.

manrajgrover commented 5 years ago

@krischer AppVeyor build is failing. Can you take a look into the issue?

manrajgrover commented 4 years ago

@krischer Ping! I wish to merge this quickly. Kindly update me when you fix this.

manrajgrover commented 4 years ago

@krischer Thanks for working on this. I've released it in 0.0.27!