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.

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

It doesn't actually depend on rails, just railties #97

Merged
merged 3 commits into from
Feb 21, 2014
Merged

It doesn't actually depend on rails, just railties #97

merged 3 commits into from
Feb 21, 2014

Conversation

rosenfeld
Copy link
Contributor

This allows an application not to download the activerecord and arel gems for instance, if they don't use such ORM. As long as the application itself doesn't depend on the rails gem but railties instead.

I've actually tested this locally in my own application since I use Sequel as the ORM solution.

The only gem I depend on that still depends on rails is js-routes. Please consider applying this patch.

This allows an application not to download the activerecord and arel gems for instance, if they don't use such ORM. As long as the application itself doesn't depend on the rails gem but railties instead.
@le0pard
Copy link
Member

le0pard commented Feb 21, 2014

Hello, @rosenfeld . I think, you should add new test env to https://github.com/railsware/js-routes/blob/master/Appraisals, generate new gemspec (rake appraisal:gemfiles and rake appraisal:install) which not contain Rails and check what all tests pass without it.

@rosenfeld
Copy link
Contributor Author

Ok, I'll try to give it a try later today.

@rosenfeld
Copy link
Contributor Author

Indeed, I missed another required js-routes dependency, added by the rails gem: sprockets-rails :)

@bogdan
Copy link
Collaborator

bogdan commented Feb 21, 2014

js-routes definitely depends on actionpack. Please include it as well if it is not part of railties dependencies.

@rosenfeld
Copy link
Contributor Author

Railties already depends on actionpack: https://github.com/rails/rails/blob/master/railties/railties.gemspec#L27

@le0pard
Copy link
Member

le0pard commented Feb 21, 2014

@rosenfeld Why we need tzinfo gem in tests?

@rosenfeld
Copy link
Contributor Author

The Rails application itself seems to need it probably due to some default configs, but this is not a js-routes dependency. All existing Rails applications will either have it or won't need tzinfo. I don't think we should worry about it.

@rosenfeld
Copy link
Contributor Author

Otherwise we should change the test application defaults not to rely on tzinfo, but I think this may be harder then we may initially think it would... And more fragile too...

@le0pard
Copy link
Member

le0pard commented Feb 21, 2014

@rosenfeld As I can see activesupport from 4.x.x have in dependency tzinfo, and railties have in dependency activesupport. So we can remove it from section rails40. And maybe it no need for railties 3.x.x

@rosenfeld
Copy link
Contributor Author

I added it because some test failed without it, but maybe you're right about rails40, didn't test it without that dependency. Will do and let you know.

@rosenfeld
Copy link
Contributor Author

That's right, rails40 doesn't need it. I'll remove it and push the changes

@rosenfeld
Copy link
Contributor Author

done

@le0pard
Copy link
Member

le0pard commented Feb 21, 2014

Looks good. Need wait for @bogdan - what he will say.

@bogdan
Copy link
Collaborator

bogdan commented Feb 21, 2014

Also looks good for me. Merging in.

bogdan added a commit that referenced this pull request Feb 21, 2014
It doesn't actually depend on rails, just railties
@bogdan bogdan merged commit 09d419e into railsware:master Feb 21, 2014
@rosenfeld
Copy link
Contributor Author

Thank you :) Any estimates for next release? ;)

@rosenfeld rosenfeld deleted the patch-1 branch February 21, 2014 19:42
@le0pard
Copy link
Member

le0pard commented Feb 21, 2014

+1 for release, because master have fix for IE :)

@bogdan
Copy link
Collaborator

bogdan commented Feb 21, 2014

check 0.9.7

@rosenfeld
Copy link
Contributor Author

Thanks :-)

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.

3 participants