Sutto / rocket_pants

API building tools on top of ActionController. Also, an awesome name.
MIT License
981 stars 130 forks source link

Added support for Rails 5.0, 5.1 and 5.2 #143

Open parse opened 8 years ago

parse commented 8 years ago

Made a few changes to make RocketPants support Rails 5.

parse commented 7 years ago

Any plans to get this into master @Sutto ?

Sutto commented 7 years ago

Hey @parse, sorry I missed this - I've been ignoring this repository for silly reasons.

Does this break backwards compat? Travis is broken so I can't check there, but will try to fix Travis ASAP to make sure I know.

If not broken, I'll merge it in asap and release.

Thank you for contributing - I appreciate it and really regret being pretty bad at running this. I'm looking to add some more people to help manage it.

parse commented 7 years ago

I have not tested it on rails 4 sorry. I've been using it on my rails 5 setup for about a year now so it's been pretty stable.

Awesome that you are coming back to this now!

sevenseacat commented 7 years ago

I've been using this fork while upgrading to Rails 5, and it throws one deprecation warning when running tests (over and over again):

DEPRECATION WARNING: Using positional arguments in functional tests has been deprecated,
in favor of keyword arguments, and will be removed in Rails 5.1.

Deprecated style:
get :show, { id: 1 }, nil, { notice: "This is a flash message" }

New keyword style:
get :show, params: { id: 1 }, flash: { notice: "This is a flash message" },
  session: nil # Can safely be omitted.
 (called from process at /Users/becky/.gem/ruby/2.3.3/bundler/gems/rocket_pants-a87933747c0f/lib/rocket_pants/test_helper.rb:107)

This is being thrown in non-Rocket-Pants-related controller specs, when the request in question doesn't actually have any parameters, ie. get :new.

If I add empty params (eg. get :new, params: {}) or remove the RocketPants test helper, the deprecation notice goes away.

Sutto commented 7 years ago

Presumably something having issues with our implementation of the test helpers (get etc) in that case?

sevenseacat commented 7 years ago

Yeah - the process method in the test helper instantiates the params argument to add the version param if necessary. There's already clauses in there for Rails 3 and Rails 4; a separate one might need to be added for Rails 5.

parse commented 7 years ago

I'm aware of the deprecations being logged in 5.0. In 5.1rc1 they have been removed permanently so it completely fails tests.

Currently don't have time to dig into this, so if anyone is up for it, I would love to get a PR on it and try to land this into master.

parse commented 7 years ago

@sevenseacat The tests have now been fixed for Rails 5. Enjoy!

iujlaki commented 7 years ago

👍

MatayoshiMariano commented 7 years ago

Will this PR be merged?

avitus commented 7 years ago

Would love to get this merged in. After looking through all the various replacement options now that I've switched to Rails 5.1, I still massively prefer this gem.

Sutto commented 7 years ago

Does this still work for Rails 4 and 3? 3 is less of a concern, but Rails 4 should be maintained.

If w can make sure it works on that and doesn't break other stuff, I'm happy to merge it in.

MatayoshiMariano commented 7 years ago

@Sutto how can we asure that? Fixing the tests?

parse commented 7 years ago

@avitus I submitted a PR here: https://github.com/Sutto/api_smith/pull/12

parse commented 7 years ago

Support for Rails 5.1 was kindly added by @MatayoshiMariano

parse commented 6 years ago

This now works for Rails 5.2 as well. @Zhong-z kindly added support for Bugsnag v6.

@Sutto Any chance we can get this merged and cut a new major version perhaps to avoid breaking support for Rails 4. (FYI, I am not using Rails 4, but I haven't heard about any issues on it for people using this fork)