search

taylorthurlow / panda-motd

a utility for generating a more useful MOTD
MIT License
134 stars 6 forks source link

Refactor #16

Closed Br1ght0ne closed 5 years ago

Br1ght0ne commented 6 years ago

The error in tests regarding ssl_certificates_spec.rb has been addressed at #17.

taylorthurlow commented 6 years ago

Just reviewing the changes, thanks for doing all of this. I'm not so sure on the Ruby version bump though. I had it at 2.2.10 because I wanted to provide some level of back support for older Ruby versions. That said, I know that 2.2 is basically unsupported, so if anything it should be a 2.3.x release.

Perhaps it's the best idea to leave .ruby-version at 2.5.1 and just let TravisCI alert when we make a breaking change for 2.4.x or 2.3.x. Thoughts?

Br1ght0ne commented 5 years ago

I think you're right. .ruby-version should be the latest supported one, not the oldest supported. TravisCI should check all major versions starting with 2.3, so we don't break anything. Good thinking!

taylorthurlow commented 5 years ago

I like all of your changes, but builds fail because delete_suffix wasn't implemented until Ruby 2.5. We should switch to something compatible with 2.3 onwards.

Br1ght0ne commented 5 years ago

Ah, I didn't see that, sorry. I think we can go with String#sub then.

taylorthurlow commented 5 years ago

Running into some issues with this merge, so I hard reset the develop branch back to where it was before the merge. Trying to sort out the fact that there are a bunch of these old merge commits attached to this PR - it creates a bunch of merge bubbles that I don't want in git history. Going to try to rebase the relevant changes onto the top of develop.

Br1ght0ne commented 5 years ago

About the merge problem: yeah, try a rebase -i on master and drop the merge commits. I think that's how it works...

taylorthurlow commented 5 years ago

I moved develop back to the point before you made any changes, and then rebased your changes onto develop. This seems to have done the trick. I did this all in the refactor branch then merged it.

Not exactly sure how to avoid this problem in the future - is it possible that this was caused by merging the develop changes into your refactor branch?

Br1ght0ne commented 5 years ago

@taylorthurlow It probably was. Though I'm not exactly sure too :)