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 version constraint for jsonapi-renderer #2189

Merged

Conversation

tagliala
Copy link

Purpose

Update 'jsonapi-renderer' dependency to meet last released version

Changes

Just the 'jsonapi-renderer' entry in the gemspec file

Caveats

Related GitHub issues

#2057

Additional helpful information

Tested against Rails: 5.1.4, 5.0.6, 4.2.9, 4.1.16

@tagliala
Copy link
Author

Failures depend on a RuboCop offence which was not introduced in this PR:

https://travis-ci.org/rails-api/active_model_serializers/jobs/276495964#L789

Offenses:
lib/active_model/serializer.rb:340:30: C: Style/SpaceInsideBlockBraces: Space inside empty braces detected.
      return Enumerator.new { } unless object
                             ^
157 files inspected, 1 offense detected

@@ -42,7 +42,7 @@ Gem::Specification.new do |spec|
# 'minitest'
# 'thread_safe'

spec.add_runtime_dependency 'jsonapi-renderer', ['>= 0.1.1.beta1', '< 0.2']
spec.add_runtime_dependency 'jsonapi-renderer', '~> 0.2.0'
Copy link
Member

Choose a reason for hiding this comment

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

What's different from the 0.1.x series to 0.2.x? This looks like it may be a breaking change and should just be changing < 0.2 to < 0.3

Copy link
Author

@tagliala tagliala Sep 17, 2017

Choose a reason for hiding this comment

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

Hi,

unfortunately they don't release a changelog :\

I've just noticed that my bundle is out of date according to bundle outdated.

I've checked some commits, but I'm not confident in saying if there is a breaking change or not: https://github.com/jsonapi-rb/jsonapi-renderer/commits/master

I could go for ['>= 0.1.1.beta1', '< 0.3'] if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Author here – it is safe to upgrade the dependency as 1. the 0.2 series is backwards compatible with the 0.1 series and 2. no AMS-used code was changed.

@bf4
Copy link
Member

bf4 commented Sep 17, 2017 via email

@tagliala tagliala force-pushed the update-jsonapi-renderer-dependency branch from 1ccc6ba to edbfac0 Compare September 17, 2017 18:35
@tagliala
Copy link
Author

PR updated

@@ -42,7 +42,7 @@ Gem::Specification.new do |spec|
# 'minitest'
# 'thread_safe'

spec.add_runtime_dependency 'jsonapi-renderer', ['>= 0.1.1.beta1', '< 0.2']
spec.add_runtime_dependency 'jsonapi-renderer', ['>= 0.1.1.beta1', '< 0.3']
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same as ~> 0.2.0, except it also allows the 0.1 series. Any specific reason?

Copy link
Author

Choose a reason for hiding this comment

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

@beauby maybe I didn't get this comment: #2189 (review)

in response to: #2189 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what @bf4 meant there either (:

Copy link
Member

Choose a reason for hiding this comment

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

@beauby I thought it would be a breaking change to AMS to change the minimum jsonapi-renderer version. Am I wrong there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right – yeah then ['>= 0.1.1.beta1', '< 0.3'] LGTM.

Copy link
Author

Choose a reason for hiding this comment

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

Should I do something else on my side?

Copy link
Contributor

Choose a reason for hiding this comment

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

@taglia No, I think we're good. @bf4?

@bf4
Copy link
Member

bf4 commented Sep 19, 2017

@tagliala Are you comfortable rebasing against 0-10-stable and force pushing to your branch?

@tagliala tagliala force-pushed the update-jsonapi-renderer-dependency branch from edbfac0 to 1c9214d Compare September 19, 2017 14:58
@tagliala
Copy link
Author

Done, everything is green 💪

@bf4 bf4 merged commit 715a702 into rails-api:0-10-stable Sep 19, 2017
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.

None yet

3 participants