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

Removed all dependencies on the Addressable gem #20

Merged
merged 1 commit into from Aug 29, 2018

Conversation

dstotz
Copy link
Contributor

@dstotz dstotz commented Aug 29, 2018

This change allows the addressable gem to be removed from our Ruby client to avoid conflicts with other gems that rely on a newer version of addressable


This change is Reviewable

@dstotz
Copy link
Contributor Author

dstotz commented Aug 29, 2018

Forgot to add one test and dropped coverage down. Adding the test now.

@coveralls
Copy link

coveralls commented Aug 29, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling d3f82e0 on dstotz:remove-addressable into 61a8a68 on springboardretail:master.

@dstotz dstotz force-pushed the remove-addressable branch 2 times, most recently from 965b891 to f64846b Compare August 29, 2018 18:11
Copy link
Contributor

@jstotz jstotz left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dstotz)


lib/springboard/client/uri.rb, line 107 at r1 (raw file):

      end

      def normalized_query_hash_values(value)

This should probably be called normalize_query_value. The verb tense should match normalize_query_hash and it should be singular because it normalizes a single value.


spec/springboard/client_spec.rb, line 91 at r1 (raw file):

    end

    it "should return a resource object when given a path as asymbol" do

typo: "asymbol" -> "a symbol"

Also, this test and the one before it look like they're not super useful / corret. Test says "should return a resource object with the given path and client" but it's only checking the class.


spec/springboard/client/resource_spec.rb, line 320 at r1 (raw file):

    end

    describe 'normalize_uri' do

This is a private method and should not be tested directly. Make sure the behavior these tests exercise is part of the test for a public method and then just delete them.


spec/springboard/client/uri_spec.rb, line 117 at r1 (raw file):

    it "should support hash param values" do
      uri.__send__(:query_values=, {:a => {:b => {:c => 123}}})

This is a public method so you don't need to use __send__


spec/springboard/client/uri_spec.rb, line 127 at r1 (raw file):

    it "should return the current query values" do
      uri.query = 'sort[]=f1&sort[]=f2&per_page=all'
      uri.__send__(:query_values=, uri.query_values)

Same here


spec/springboard/client/uri_spec.rb, line 132 at r1 (raw file):

  end

  describe 'normalize_query_hash' do

Another private method that shouldn't be tested directly.


spec/springboard/client/uri_spec.rb, line 149 at r1 (raw file):

  end

  describe 'normalized_query_hash_values' do

Another private method that shouldn't be tested directly.

Copy link
Contributor

@jstotz jstotz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dstotz)


lib/springboard/client/resource.rb, line 174 at r1 (raw file):

      # @return [Resource]
      def embed(*embeds)
        embeds = (query['_include[]'] || []) + embeds

Since you had to make this change, don't we have to make a similar change in Collection#filter? I don't like the asymmetry of writing as key and reading as key[]. Seems like it's just asking for bugs. Maybe we should strip off the [] when generating a hash in URI#query_values.

Copy link
Contributor Author

@dstotz dstotz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 7 files reviewed, 8 unresolved discussions (waiting on @jstotz and @dstotz)


lib/springboard/client/resource.rb, line 174 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

Since you had to make this change, don't we have to make a similar change in Collection#filter? I don't like the asymmetry of writing as key and reading as key[]. Seems like it's just asking for bugs. Maybe we should strip off the [] when generating a hash in URI#query_values.

Good call. I've made that change and fixed the tests.


lib/springboard/client/uri.rb, line 107 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

This should probably be called normalize_query_value. The verb tense should match normalize_query_hash and it should be singular because it normalizes a single value.

Done.


spec/springboard/client_spec.rb, line 91 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

typo: "asymbol" -> "a symbol"

Also, this test and the one before it look like they're not super useful / corret. Test says "should return a resource object with the given path and client" but it's only checking the class.

Done.


spec/springboard/client/resource_spec.rb, line 320 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

This is a private method and should not be tested directly. Make sure the behavior these tests exercise is part of the test for a public method and then just delete them.

I have moved tests for this behavior into the [] method on the client spec as that is when this method gets called


spec/springboard/client/uri_spec.rb, line 117 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

This is a public method so you don't need to use __send__

Done.


spec/springboard/client/uri_spec.rb, line 127 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

Same here

Done.


spec/springboard/client/uri_spec.rb, line 132 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

Another private method that shouldn't be tested directly.

Moved relevant tests to query_values= tests


spec/springboard/client/uri_spec.rb, line 149 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

Another private method that shouldn't be tested directly.

Moved relevant tests to query_values= tests

Copy link
Contributor

@jstotz jstotz left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jstotz)


lib/springboard/client/uri.rb, line 107 at r1 (raw file):

Previously, dstotz (Derek Stotz) wrote…

Done.

Only partially addresses my suggestion

Copy link
Contributor Author

@dstotz dstotz left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jstotz)


lib/springboard/client/uri.rb, line 107 at r1 (raw file):

Previously, jstotz (Jay Stotz) wrote…

Only partially addresses my suggestion

Sorry about that, totally forgot to remove the d. Just pushed an update.

Copy link
Contributor

@jstotz jstotz left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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