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

Update codebase to support latest AMS version. #54

Merged
merged 5 commits into from
Jul 15, 2016

Conversation

drn
Copy link
Member

@drn drn commented Jul 13, 2016

There were a number of changes in the current ActiveModelSerializers versions that broke the functionality of this library. These changes should fix them.

This addresses @dblock's comment raised here #53 and should solve this outstanding issue #52

Specs were failing for ruby v2.1 because the latest rack requires >= ruby v2.2.2. Gem::InstallError: rack requires Ruby version >= 2.2.2. I pushed up a commit to make this pull green, but I'm not sure what the best approach is: dropping ruby 2.1 support or locking rack. Let me know if you'd like me to swap this.

@drn drn force-pushed the apply_adapter_config_value branch from d72ac82 to 346b7ed Compare July 13, 2016 05:38
@drn drn changed the title Update codebase to support work with latest AMS. Update codebase to support latest AMS. Jul 13, 2016
@drn drn changed the title Update codebase to support latest AMS. Update codebase to support latest AMS changes. Jul 13, 2016
@drn drn changed the title Update codebase to support latest AMS changes. Update codebase to support latest AMS version. Jul 13, 2016
@drn drn force-pushed the apply_adapter_config_value branch from 4723411 to c0234c9 Compare July 13, 2016 07:42
@@ -2,6 +2,7 @@

### 1.4.0 (Next)

* [#54](https://github.com/ruby-grape/grape-active_model_serializers/pull/54): Handle breaking changes of current AMS version - [@drn](https://github.com/drn).
Copy link
Member

Choose a reason for hiding this comment

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

This should say "Added support for AMS 5.0" I believe.

@dblock
Copy link
Member

dblock commented Jul 13, 2016

Ok this is great. If I understand correct this is backward compatible with AMS 4.x? I'd like some mention of what version(s) of AMS we support in README, please. If not we should do a 1.4.0 release with all the changes before merging this, first.

You can just remove ruby 2.1 builds altogether, unless we want to make extra effort to support that. I am fine with not.

@drn
Copy link
Member Author

drn commented Jul 13, 2016

Whoops, I should've updated the gemspec to reflect this, but these changes are not backwards compatible with ASM v0.9.x. These changes work only with the 0.10.x releases. Being backwards compatible with v0.9.x we could potentially do, but it'd require a significant amount of work. Are you okay with dropping support for v0.9.x?

It looks like there's a breaking change in v1.4.0 already, so I'm happy to help release v1.4.0 before this gets merged in. Since this is another breaking change, I'm assuming that we'd want this to be v1.5.0.

I was also considering having travis explicitly test ASM versions. I was thinking of including the 0.10.0, 0.10.1, 0.10.2, and HEAD ASM versions in the travis build matrix permutations, but if we do that against ruby 2.3.1 and each of the 9 grape versions that already there, it would add 24 additional builds. Thoughts?

@drn drn force-pushed the apply_adapter_config_value branch from c0234c9 to d0fb1ff Compare July 13, 2016 21:25
This was referenced Jul 13, 2016
Closed
@drn drn force-pushed the apply_adapter_config_value branch 2 times, most recently from 0cef317 to ef068e5 Compare July 13, 2016 23:07
@dblock
Copy link
Member

dblock commented Jul 14, 2016

If the work to support ASM 0.9.x is too big, I say we don't do it.

I think your plan is good. Do a release of 1.4.0 with the changes on master, then feel free to merge this and do a 1.5.0 when that's all ready at will.

@drn
Copy link
Member Author

drn commented Jul 14, 2016

Great, I will do a release of v1.4.0 today and handle v1.5.0

@drn drn force-pushed the apply_adapter_config_value branch from ef068e5 to 6b4d148 Compare July 15, 2016 02:18
@drn drn force-pushed the apply_adapter_config_value branch from 6b4d148 to 8f1687b Compare July 15, 2016 02:26
@drn drn merged commit fb34024 into ruby-grape:master Jul 15, 2016
@drn drn deleted the apply_adapter_config_value branch July 15, 2016 02:28
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