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.

Sign up for GitHub

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

Jump to bottom

Will update gems #144

Merged
merged 9 commits into from May 2, 2014
Merged

Will update gems #144

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
96d4fdc
Update dependecy to match other files.
Apr 9, 2014
1524bc8
Bump version here, as it has been bumped in the gemspec.
Apr 9, 2014
06329a2
Read version from file, rather than hard coding.
Apr 25, 2014
44b26bd
Read Version from file for collins-shell
Apr 25, 2014
2969489
Read version from file for collins_notify.
Apr 25, 2014
db0e678
Read version from file for collins_state.
Apr 25, 2014
3281c1a
Instead of just reading the VERISON file, read the first line, and st…
Apr 25, 2014
a5fdc76
Don't worry about whitespace, since gemspec seems to handle it correc…
Apr 25, 2014
dd568ba
Removing rvmrc files. Let people manage their own ruby envs
Apr 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion support/collins-shell/Rakefile
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ jeweler = Jeweler::Tasks.new do |gem|
gem.files.exclude "spec/**/*"
gem.files.exclude '.gitignore'
gem.files.exclude '.rspec'
gem.add_runtime_dependency 'collins_client', '~> 0.2.9'
gem.add_runtime_dependency 'collins_client', '~> 0.2.10'
gem.add_runtime_dependency 'highline', '~> 1.6.15'
gem.add_runtime_dependency 'mustache', '~> 0.99.4'
gem.add_runtime_dependency 'pry', '~> 0.9.9.6'
Expand Down
2 changes: 1 addition & 1 deletion support/ruby/collins-state/VERSION
View file
Open in desktop
@@ -1 +1 @@
0.2.11
0.2.12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.12

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.)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/144/files#r11437023
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the gemspec doesnt read that file in, then yea. I dont know what purpose it serves. @dallasmarlow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@byxorna @Primer42 agreed, as much as i like blake this is an artifact of his that should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dallasmarlow @byxorna how does this PR look now? Reading the version from the file, rather than having it hard coded in a few places?