Will update gems #144
Will update gems #144
Conversation
This file had collins_client set at 0.2.9, but the others had 0.2.10, so I'm bumping that dependency.
| 0.2.12 |
There was a problem hiding this comment.
Can we remove this file? No need for it anymore, we dont read from it after jeweler was ripped out. (dont forget to remove from gemspec files list as well.)
There was a problem hiding this comment.
All of the gems have it - are you comfortable with us removing it from all
of them?
-William Richard
On Wed, Apr 9, 2014 at 10:11 AM, Gabe Conradi notifications@github.comwrote:
In support/ruby/collins-state/VERSION:
\ No newline at end of file
+0.2.12Can we remove this file? No need for it anymore, we dont read from it
after jeweler was ripped out. (dont forget to remove from gemspec files
list as well.)—
Reply to this email directly or view it on GitHubhttps://github.com//pull/144/files#r11437023
.
There was a problem hiding this comment.
If the gemspec doesnt read that file in, then yea. I dont know what purpose it serves. @dallasmarlow
There was a problem hiding this comment.
@byxorna @Primer42 agreed, as much as i like blake this is an artifact of his that should be removed.
There was a problem hiding this comment.
The version file does not appear to be totally unused in all cases. For collins_shell, and collins_notify, blake appears to have used them
https://github.com/tumblr/collins/blob/master/support/collins-shell/lib/collins_shell/cli.rb#L149
https://github.com/tumblr/collins/blob/master/support/ruby/collins-notify/lib/collins_notify/version.rb#L8
I don't know much ruby, but it seems like there are better ways to do this. Maybe something like this.
http://stackoverflow.com/questions/3046233/get-version-of-gem-within-ruby
There was a problem hiding this comment.
@Primer42 i'm a bit confused here, the page you linked to has an example of programmatically retrieving the version from the gemspec of a loaded gem. @byxorna and myself are just suggesting to keep the library version only in the gemspec to avoid these different files from conflicting. the VERSION file is currently not used in that project, if you really wanted to keep the VERSION files around for some reason then you would just want to have the gemspec read it gem.version = File.read 'VERSION'.
There was a problem hiding this comment.
@dallasmarlow @Primer42 is saying that the collins-shell and collins-notify libraries read the VERSION file from disk, so removing the VERSION file will require changes to how those libraries report their versions.
@Primer42 that suggestion is specific to rubygems. We dont want to force users of these gems to have rubygems installed, rather than another method (i.e. managing $: manually). For the CollinsNotify::Version, I would say just rip that whole class out, and remove https://github.com/tumblr/collins/blob/master/support/ruby/collins-notify/lib/collins_notify/options.rb#L108. Not super necessary.
For collins_shell, its more integrated, and it looks like he is already using Gem to figure out what the latest gem is for comparison. Unsure what to do there.
There was a problem hiding this comment.
@byxorna yep, you summarized what I was trying to say better than I was saying it :)
Do you really think its a good idea to remove the --version option? That seems pretty standard to me, either to have that option and/or display the version information in the usage message. Either way, we need a way to access that information in some way.
I know this is sort of the opposite of what you suggested @dallasmarlow, but what if instead of having the version number hard coded in several places, we have all of these files load the version number dynamically from the VERSION file?
There was a problem hiding this comment.
@dallasmarlow @byxorna how does this PR look now? Reading the version from the file, rather than having it hard coded in a few places?
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| Gem::Specification.new do |s| | |||
| s.name = "collins_notify" | |||
| s.version = "0.0.4" | |||
| s.version = File.read 'VERSION' | |||
There was a problem hiding this comment.
Not sure how gem handles version with whitespace in it. May wanna strip what you read from VERSION.
There was a problem hiding this comment.
@byxorna I built each of these gems before I pushed them - it didn't seem to mind any white space.
There was a problem hiding this comment.
That said, I thought about it, and I think you're right @byxorna . How does it look now?
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| Gem::Specification.new do |s| | |||
| s.name = "collins_shell" | |||
| s.version = "0.2.19" | |||
| s.version = File.readlines('VERSION')[0].strip() | |||
There was a problem hiding this comment.
Was thinking just File.read('VERSION').strip
|
LGTM, I think this is in a mergable state. |
Will update gems
Will update gems
Just fixing some gem versioning stuff. Nothing complicated.
I've also pushed any updated gems to rubygems.org, so the version numbers here match the version numbers there.