[WEBSITE-899][WEBSITE-946][WEBSITE-950] Extract components to separate gems. #63

katylouise · 2017-04-19T15:39:36Z

Extracted decorators to separate gem.
Extracted NTriple related code (NTripleResponseBuilder, NTripleResponse) to separate gem.
Created a BaseResponse.
Refactored tests.
Refactored documentation.

houndci-bot · 2017-04-19T15:40:05Z

lib/parliament/request/url_request.rb

+      # @param [String] base_url the base url of our api. (expected: http://example.com - without the trailing slash).
+      # @param [Hash] headers the headers being sent in the request.
+      # @param [Parliament::Builder] builder the builder to use in order to build a response.
+      # @params [Module] decorators the decorator module to use in order to provide possible alias methods for any objects created by the builder.


Line is too long. [146/120]

houndci-bot · 2017-04-19T15:40:06Z

lib/parliament/request/base_request.rb

      #
      # @raise [Parliament::ServerError] when the server responds with a 5xx status code.
      # @raise [Parliament::ClientError] when the server responds with a 4xx status code.
      # @raise [Parliament::NoContentResponseError] when the server responds with a 204 status code.
      #
      # @param [Hash] params (optional) additional URI encoded form values to be added to the URI.
      #
-      # @return [Parliament::Response] a Parliament::Response object containing all of the nodes returned from the URL.
+      # @return [Parliament::Response::BaseResponse] a Parliament::Response::BaseResponse object containing all of the data returned from the URL.


Line is too long. [146/120]

houndci-bot · 2017-04-19T15:40:06Z

lib/parliament/request/base_request.rb

      #
      # @param [String] base_url the base url of our api. (expected: http://example.com - without the trailing slash).
      # @param [Hash] headers the headers being sent in the request.
-      def initialize(base_url: nil, headers: nil, builder: nil)
+      # @param [Parliament::Builder] builder the builder to use in order to build a response.
+      # @params [Module] decorators the decorator module to use in order to provide possible alias methods for any objects created by the builder.


Line is too long. [146/120]

…arate gems. * Extracted decorators to separate gem. * Extracted NTriple related code (NTripleResponseBuilder, NTripleResponse) to separate gem. * Created a BaseResponse. * Refactored tests. * Refactored documentation.

mattrayner

Hi @bex, I've added some comments/changes below

mattrayner · 2017-04-19T16:15:11Z

Gemfile

@@ -5,3 +5,6 @@ gemspec

 # Include coveralls for CI coverage reports
 gem 'coveralls', require: false
+
+gem 'parliament-grom-decorators', path: '../parliament-grom-decorators'


If these are being used for testing we will need to either add them as development dependencies or look at how they are needed within the app.

This is also breaking tests

mattrayner · 2017-04-19T16:16:05Z

lib/parliament/builder/base_response_builder.rb

@@ -1,12 +1,21 @@
 module Parliament
  module Builder
+    # API response builder, allowing the user to return the body of an HTTPResponse.


Should this be 'Base response builder'? It's not technically API specific

mattrayner · 2017-04-19T16:16:32Z

lib/parliament/builder/base_response_builder.rb

    class BaseResponseBuilder
-      def initialize(response)
+      # Creates a new BaseReponseBuilder.
+      # @param [HTTPResponse] response an HTTP response containing n-triple data.


Does it need to contain ntriple data?

mattrayner · 2017-04-19T16:18:20Z

lib/parliament/builder/base_response_builder.rb

-      def initialize(response)
+      # Creates a new BaseReponseBuilder.
+      # @param [HTTPResponse] response an HTTP response containing n-triple data.
+      # @param [Module] decorators the decorator modules to provide alias methods to the resulting objects.


Should this be more like:

'A namespace which contains modules used to decorate the objects we receive'?

Infact, is this even needed? Our base response doesn't actually use the decorators?

Seeing the code below perhaps we need to mention why it's not used and that it is for API completeness?

mattrayner · 2017-04-19T16:20:11Z

lib/parliament/request/base_request.rb

@@ -1,78 +1,83 @@
 module Parliament
  module Request
+    # API request object, allowing the user to make a request to an API.


Is this API specific?

mattrayner · 2017-04-19T16:37:48Z

spec/parliament/request/base_request_spec.rb

@@ -61,10 +61,10 @@
    end

    context 'it accepts query parameters' do


What is this actually testing?

How does this test prove that what you're expecting to happen, actually happens? You don't appear to be checking that the params hash actually generates the URL you expect? Just that a connection to that URL could be made. Infact, this response isn't even checking that it's a 200?

I have changed this test to check the request made contains the parameters expected in the url.

mattrayner · 2017-04-19T16:38:21Z

spec/parliament/request/url_request_spec.rb

@@ -24,7 +25,7 @@
  end

  describe '#respond_to_missing?' do
-    subject { Parliament::Request::UrlRequest.new(base_url: 'http://test.com') }
+    subject { Parliament::Request::UrlRequest.new(base_url: 'http://test.com', decorators: Parliament::Grom::Decorator) }


Looking at this, parliament-grom-decorators should be aded as a development dependency?

I have added parliament-grom-decorators as a dev dependency in the gemspec.

mattrayner · 2017-04-19T16:38:54Z

spec/parliament/response/base_response_spec.rb

+  let(:response) { 'hello world' }
+  subject { Parliament::Response::BaseResponse.new(response) }
+
+  it 'has a response' do


Is this the correct way to test what a method returns?

mattrayner · 2017-04-19T16:39:52Z

spec/parliament/utils_spec.rb

@@ -22,23 +22,23 @@



Should these utils be part of the Ntriple gem? It's directly related to that data and output?

mattrayner · 2017-04-19T16:40:14Z

spec/spec_helper.rb

@@ -8,6 +8,8 @@

 $LOAD_PATH.unshift File.expand_path('../../lib', __FILE__)
 require 'parliament'
+require 'parliament/grom/decorator'


These two should DEFINITELY be development dependencies

mattrayner · 2017-04-19T16:45:09Z

On top of this, you're tests fail because of the relative paths used in Gemfile

houndci-bot · 2017-04-20T11:58:06Z

lib/parliament/request/url_request.rb

+      # @param [Module] decorators the decorator module to use in order to provide possible alias methods for any objects created by the builder.
+      # @example Passing headers
+      #
+      # request = Parliament::Request::UrlRequest.new(base_url: 'http://example.com', headers: { 'Access-Token' => '12345678' })


Line is too long. [128/120]

houndci-bot · 2017-04-20T11:58:06Z

lib/parliament/request/url_request.rb

+      # @param [String] base_url the base url of our api. (expected: http://example.com - without the trailing slash).
+      # @param [Hash] headers the headers being sent in the request.
+      # @param [Parliament::Builder] builder the builder to use in order to build a response.
+      # @param [Module] decorators the decorator module to use in order to provide possible alias methods for any objects created by the builder.


Line is too long. [145/120]

houndci-bot · 2017-04-20T11:58:06Z

lib/parliament/builder/base_response_builder.rb

-      def initialize(response)
+      # Creates a new BaseReponseBuilder.
+      # @param [HTTPResponse] response an HTTP response.
+      # @param [Module] decorators a namespace which contains modules used to decorate the objects we receive.  It is not used directly by the BaseResponseBuilder, but is there for API completeness.


Line is too long. [198/120]

coveralls · 2017-04-20T11:59:20Z

Coverage remained the same at 100.0% when pulling 3c9e889 on katylouise:katylouise/website-899_split-out-ntriples into fec78fd on ukparliament:master.

coveralls · 2017-04-20T12:12:46Z

Coverage remained the same at 100.0% when pulling f8a0ed8 on katylouise:katylouise/website-899_split-out-ntriples into fec78fd on ukparliament:master.

…an 204

coveralls · 2017-04-20T15:04:43Z

Coverage remained the same at 100.0% when pulling 513a0de on katylouise:katylouise/website-899_split-out-ntriples into fec78fd on ukparliament:master.

houndci-bot reviewed Apr 19, 2017

View reviewed changes

katylouise force-pushed the katylouise/website-899_split-out-ntriples branch from 5dda347 to b8a1951 Compare April 19, 2017 15:42

katylouise requested a review from mattrayner April 19, 2017 15:43

mattrayner suggested changes Apr 19, 2017

View reviewed changes

houndci-bot reviewed Apr 20, 2017

View reviewed changes

Added a test to url_request and updated dependencies

3c9e889

Set version of parliament-grom-decorators in gemspec

f8a0ed8

Updated no content response to match on empty response body rather th…

513a0de

…an 204

mattrayner approved these changes Apr 24, 2017

View reviewed changes

mattrayner merged commit 5a1aa34 into ukparliament:master Apr 24, 2017

katylouise deleted the katylouise/website-899_split-out-ntriples branch June 9, 2017 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WEBSITE-899][WEBSITE-946][WEBSITE-950] Extract components to separate gems. #63

[WEBSITE-899][WEBSITE-946][WEBSITE-950] Extract components to separate gems. #63

katylouise commented Apr 19, 2017

houndci-bot Apr 19, 2017

houndci-bot Apr 19, 2017

houndci-bot Apr 19, 2017

mattrayner left a comment

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

katylouise Apr 20, 2017

mattrayner Apr 19, 2017

katylouise Apr 20, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner Apr 19, 2017

mattrayner commented Apr 19, 2017

houndci-bot Apr 20, 2017

houndci-bot Apr 20, 2017

houndci-bot Apr 20, 2017

coveralls commented Apr 20, 2017

coveralls commented Apr 20, 2017

coveralls commented Apr 20, 2017

		@@ -61,10 +61,10 @@
		end

		context 'it accepts query parameters' do

[WEBSITE-899][WEBSITE-946][WEBSITE-950] Extract components to separate gems. #63

[WEBSITE-899][WEBSITE-946][WEBSITE-950] Extract components to separate gems. #63

Conversation

katylouise commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattrayner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattrayner commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 20, 2017

coveralls commented Apr 20, 2017

coveralls commented Apr 20, 2017