Drop old rubies support #700
Conversation
|
The patch is big but it's mostly indentation, better look at it with https://github.com/colszowka/simplecov/pull/700/files?w=1. |
|
I made this PR because I wanted to work on a fix, but the current CI matrix, and the fact that I might have to debug issues under a ruby version that I'm not even sure I'll be able to install made me lazy 😃. |
|
Hi there 👋 Thanks for your contribution. It's something that's definitely on our radar but I'm not sure we wanna do right now. I always saw it as a for 1.0 thing to remove them but we might as well. I'm a bit surprised to see 2.2 go already (where has time been?). I'm a bit scared about this as it will basically break all other PRs most likely. But getting rid off 1.8 support things etc. is great and has been long coming. As you already did work that the other day @colszowka and I talked about briefly. So I'm 👍 (haven't reviewed all of it yet). What do you think @colszowka @bf4 |
| env: JRUBY_OPTS=--debug | ||
|
|
||
| - rvm: jruby-head | ||
| env: JRUBY_OPTS=--debug |
There was a problem hiding this comment.
I think we say this is needed but I'm not sure it is anymore :)
There was a problem hiding this comment.
yeah you're right, tests used to pass without it but then 🤷♂️ might check in with the jruby folks again
README.md
Outdated
| does not make your test suite crash right now. | ||
|
|
||
| SimpleCov is built in [Continuous Integration] on Ruby 1.9.3, 2.0.0, 2.1, 2.2, 2.3, 2.4, 2.5 as well as JRuby 9.1. | ||
| SimpleCov is built in [Continuous Integration] on Ruby 2.3, 2.4, 2.5 as well as JRuby 9.1 and 9.2. |
There was a problem hiding this comment.
I think we could add that it should work on ruby versions 2+ but not guaranteed. Although I'm still not sure removing support is the best thing there/if we want to support maybe 2.1/2.2 or even 2.0
There was a problem hiding this comment.
Sure, if we decide to stop testing against those version but not bump the required_ruby_version setting in the gemspec, then it could be a good note to add. I did bump the setting though in this PR, since I think life is easier when you test your gem against every ruby you allow it to install against.
There was a problem hiding this comment.
Definitely, I'd tentatively also put support back in for 2+. Albeit I think required kwargs 2.1 so that's something I'd like to take along :)
Also, while many won't believe it a lot of people are running outdated rubies and I don't wanna take that storm easily :D
There was a problem hiding this comment.
Ok, so you're suggesting to put back support for 2.0, 2.1 and 2.2, but stop testing against them?
Also, while many won't believe it a lot of people are running outdated rubies and I don't wanna take that storm easily :D
Dropping support for old rubies should break nobody's code, it will just prevent installation of newer versions of the gem on those rubies. Bundler will resolve to the newest version supporting the user's ruby version. This seems unclear so I wanted to make it explicit.
There was a problem hiding this comment.
I know it's not gonna break code, but sadly people have a history of not being too happy with it although imo it's perfectly fine for them to use older gem versions as they're the ones on an old ruby version or on an old version of a dependency. People don't tend to easily agree with me :)
I'm not suggesting putting it back in, it's my thoughts at the moment. I'd wait for feedback for @colszowka what his thoughts are. My vote currently would be 2.1+ (so that I get required kwargs) but while we drop support once we might as well just go all out and remove all EOL'ed rubies as this PR suggests. Upgrading rubies in the 2.x series is usually very easy anyhow.
There was a problem hiding this comment.
I know it's not gonna break code, but sadly people have a history of not being too happy with it although imo it's perfectly fine for them to use older gem versions as they're the ones on an old ruby version or on an old version of a dependency. People don't tend to easily agree with me :)
Ok, I'm happy it's clear then :) I guess people have very different situations in the wild, but to me it's very weird that upgrading to the latest simplecov takes priority over stopping using ruby versions that don't get security patches applied. And even more weird to get mad at simplecov maintainers for not letting them do that 🤣.
I'm not suggesting putting it back in, it's my thoughts at the moment. I'd wait for feedback for @colszowka what his thoughts are. My vote currently would be 2.1+ (so that I get required kwargs) but while we drop support once we might as well just go all out and remove all EOL'ed rubies as this PR suggests. Upgrading rubies in the 2.x series is usually very easy anyhow.
Sounds good to me! Once you have a final decision I can update this PR accordingly 👍.
Thanks!
| false | ||
| end | ||
| end | ||
|
|
| end | ||
| end | ||
| end | ||
| end |
|
I'm aware this was planned for 1.0, but at this point we don't know when 1.0 will happen, so I figured we could do it now. It's an open debate whether dropping support for EOL'd rubies warrants a major version bump (see for example the discussion at https://github.com/rspec/rspec/issues/25), but in this case, being at 0.x, I think no one will get mad :) Regarding this PR breaking other PRs, in principle, it would seem to me this would make it easier for other PRs to get to green since they'll need to support less edge cases. Do you mean it will introduce merge conflicts? If that's the case, I have no problem helping with the rebases when needed. |
|
Thanks so much for having a look at this, by the way! ❤️ |
|
@deivid-rodriguez yup was about merge conflicts sorry for not being clear :) Thanks for appreciation, I should have taken a look at way more PRs but you know live and all. 😁 |
|
Ping @colszowka. Any thoughts on this? |
|
#705 generated a bunch of conflicts on this PR, so I rebased it! |
|
Hei! Some months have passed and I saw @colszowka merged one PR! 🎉 Any news here? 😃 |
|
Hey @deivid-rodriguez - thanks for keeping all this up! 💚 I took a bit of a break from simplecov, it's not exactly the easiest project to maintain 😅 Life and all. I'll reach out to @colszowka via email and stuff again to get approval for this. If I merge stuff inbetween I'll also try to take care of the merge conflicts our hold. I absolutely wanna do this but do not feel like I have the authority to decide so by myself. Thanks for being so patient, nice and amazing to do this and keep it updated.
|
|
No problem, I totally understand. I don't mind rebasing this one more time if it helps! |
|
@deivid-rodriguez that'd be great, let me try to get word from Christoph first. And then my thinking was to first do one "last" release with support for all the old rubies and then wipe them clean/merge this PR> |
|
Hey everyone, sounds reasonable to me +1 :) |
|
Thanks for chiming in @colszowka 🎉 💚
|
|
@deivid-rodriguez most of your changes affect spec file (and are basically indentation) right? I'm not missing anything big (jruby_fix and some other small stuff not considering). I'm just trying to evaluate whether to merge this first or #694 but this makes me feel like this would be a better candidate to go in first :) Any thoughts on that? |
|
Hi @PragTob! My changes are indeed mostly indentation. The reason is that many specs were wrapped with a I'm going to rebase this PR now. Regarding which one is best, I don't know how hard would be to rebase the PR you referenced on top of this one, but I would say it's better to merge this one first, because the "support target" for that feature would be greatly simplify if we drop support first. |
Since their support range matches simplecov's.
|
@deivid-rodriguez yeah that was my thinking as well, I double checked the the changed files and there didn't seem to be too much overlap. JRuby failed on Travis, restarting.... |
|
Rebased and put on master. Thank you so much @deivid-rodriguez !
|
Thoughts on dropping support for EOL'd rubies? I think it's a good idea to take advantage of being still on 0.x and drop support on 0.17.0, for example.
I just did this quick pass since I'm not sure if you'll be interested. If you are, I can update the change log and make any other extra changes.