Skip to content

Conversation

@pjfanning
Copy link
Contributor

}

ext {
jacksonVersion = '2.9.6'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

compile 'com.fasterxml.jackson.core:jackson-core:2.4.2'
compile 'com.fasterxml.jackson.core:jackson-annotations:2.4.2'
compile 'com.fasterxml.jackson.core:jackson-databind:2.4.2'
compile "com.fasterxml.jackson.core:jackson-core:$jacksonVersion"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor issue here with the quotes using double quotes instead of single quotes that are used elsewhere. For consistency let's use single quotes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the double quotes are necessary to allow string interpolation to happen - single quotes mean the text is literal

http://docs.groovy-lang.org/latest/html/documentation/#_string_interpolation

Copy link
Contributor

@thewheat thewheat left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR @pjfanning

Minor thing of inconsistent quotes (using double instead of single which is used in the rest of the file)

Once we get that fixed I can get the team to review this and get it merged as soon as possible 👍

@pjfanning
Copy link
Contributor Author

@thewheat is the answer that I provided to your change request ok?

@thewheat
Copy link
Contributor

So sorry @pjfanning I missed the notification of your reply. Since we need double quotes for that, could we switch to double quotes for the entire file then? Just for consistency 👍

@pjfanning
Copy link
Contributor Author

@thewheat I updated the pull request to use double quotes for all dependencies

@thewheat
Copy link
Contributor

Fantastic @pjfanning! I'll get the team to verify that this is all good and we can merge it 👍 Thank you so much!

Copy link
Contributor

@SeanHealy33 SeanHealy33 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 @pjfanning

@thewheat thewheat merged commit 3600127 into intercom:master Jul 17, 2018
@pjfanning pjfanning deleted the jackson-upgrade branch July 17, 2018 11: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