Skip to content

General improvements#15

Merged
calonso merged 8 commits intomasterfrom
improve_load_style
Jan 20, 2017
Merged

General improvements#15
calonso merged 8 commits intomasterfrom
improve_load_style

Conversation

@calonso
Copy link
Copy Markdown
Owner

@calonso calonso commented Jan 12, 2017

@calonso
Copy link
Copy Markdown
Owner Author

calonso commented Jan 12, 2017

Any comment anyone? I'll leave it here a couple of days just in case.

@elhu
Copy link
Copy Markdown
Collaborator

elhu commented Jan 12, 2017

I'll have a quick look in the morning!

@calonso
Copy link
Copy Markdown
Owner Author

calonso commented Jan 17, 2017

Hi @elhu, did you had time to have a look at this?

Copy link
Copy Markdown
Collaborator

@elhu elhu left a comment

Choose a reason for hiding this comment

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

Just a couple questions/comments, everything looks good to me otherwise.


Gem::Specification.new do |spec|
spec.name = "ruby-push-notifications"
spec.version = RubyPushNotifications::VERSION
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just curious, what's the motivation behind removing the version file?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

To avoid having to require the gem's files in this file. It is a recommendation I took from ruby gems.org

Comment thread ruby-push-notifications.gemspec Outdated
spec.test_files = spec.files.grep(%r{^(test|spec|features)/})
spec.require_paths = ['lib']

spec.add_dependency 'activesupport', '~> 4.2'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we depend on activesupport exactly?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I think it is a dependency that builder gem brings. I had to fix it because the current version is 5.x and it is not compatible with Ruby <= 2.1.x

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm, weird, there's nothing about activesupport in builder's dependencies: https://rubygems.org/gems/builder.

I think forcing the requirement here is a bad idea, since projects using rails 5 depend on activesupport ~> 5.0.

If it's for test purpose, we need to have a build matrix with different version of activesupport depending on the ruby version.
I did something like that in https://github.com/elhu/peastash.

If we actually need it at runtime and not just in specs, we need to:

  • Figure out where it comes from
  • See if we can't just leave the dependency there and let bundler sort it out

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Ok, so I've just discovered it is a factory_girl's dependency so I assume we should do the build matrix you suggest

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Then it might be simpler to just register the dependency as a dev-time dependency, no?

spec.add_development_dependency 'activesupport', '~> 4.2'

This way, this requirement is for dev only and has no impact on people trying to add the gem to their projects?

@calonso calonso force-pushed the improve_load_style branch from c539eca to 38270b4 Compare January 18, 2017 17:08
@calonso calonso force-pushed the improve_load_style branch from 38270b4 to d147478 Compare January 18, 2017 17:30
@calonso
Copy link
Copy Markdown
Owner Author

calonso commented Jan 18, 2017

Cool, this looks much better. /cc @elhu

Copy link
Copy Markdown
Collaborator

@elhu elhu left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@calonso calonso merged commit d89cb59 into master Jan 20, 2017
@calonso calonso deleted the improve_load_style branch January 20, 2017 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants