Drop Ruby 1.8 support and clean up by HashNotAdam · Pull Request #66 · fog/fog-xenserver

HashNotAdam · 2017-02-24T05:53:07Z

Remove Ruby 1.8 Travis CI tests
Remove json gem (only needed for Ruby 1.8)
Fix style guide violations
Reduce duplication
Reduce complexity

TODO

Fog::Compute::XenServer::Models::ClassMethods.define_methods has a note about how it could be better designed if not for Ruby 1.8.7 but I couldn't see a test that could explain the use case. Hopefully someone else can help with this.

…argument

houndci-bot · 2017-02-24T05:53:24Z

    storage_repository_class.require_before_save.must_equal([ :name, :type ])
  end
-end
+


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :type => :type,
+                                              :uuid => :uuid,
+                                              :virtual_allocation => :virtual_allocation,
+                                              :pbds => :PBDs,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :tags => :tags,
+                                              :type => :type,
+                                              :uuid => :uuid,
+                                              :virtual_allocation => :virtual_allocation,


Use the new Ruby 1.9 hash syntax.
Line is too long. [89/80]

houndci-bot · 2017-02-24T05:53:25Z

+                                              :sm_config => :sm_config,
+                                              :tags => :tags,
+                                              :type => :type,
+                                              :uuid => :uuid,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :shared => :shared,
+                                              :sm_config => :sm_config,
+                                              :tags => :tags,
+                                              :type => :type,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :description => :description,
+                                              :introduced_by => :introduced_by,
+                                              :local_cache_enabled => :local_cache_enabled,
+                                              :name => :name,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :current_operations => :current_operations,
+                                              :description => :description,
+                                              :introduced_by => :introduced_by,
+                                              :local_cache_enabled => :local_cache_enabled,


Use the new Ruby 1.9 hash syntax.
Line is too long. [91/80]

houndci-bot · 2017-02-24T05:53:25Z

+                                              :content_type => :content_type,
+                                              :current_operations => :current_operations,
+                                              :description => :description,
+                                              :introduced_by => :introduced_by,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :blobs => :blobs,
+                                              :content_type => :content_type,
+                                              :current_operations => :current_operations,
+                                              :description => :description,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:25Z

+                                              :allowed_operations => :allowed_operations,
+                                              :blobs => :blobs,
+                                              :content_type => :content_type,
+                                              :current_operations => :current_operations,


Use the new Ruby 1.9 hash syntax.
Line is too long. [89/80]

houndci-bot · 2017-02-24T05:53:26Z

    storage_repository_class.require_before_save.must_equal([ :name, :type ])
  end
-end
+


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2017-02-24T05:53:26Z

+                                              :type => :type,
+                                              :uuid => :uuid,
+                                              :virtual_allocation => :virtual_allocation,
+                                              :pbds => :PBDs,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:26Z

+                                              :tags => :tags,
+                                              :type => :type,
+                                              :uuid => :uuid,
+                                              :virtual_allocation => :virtual_allocation,


Use the new Ruby 1.9 hash syntax.
Line is too long. [89/80]

houndci-bot · 2017-02-24T05:53:26Z

+                                              :sm_config => :sm_config,
+                                              :tags => :tags,
+                                              :type => :type,
+                                              :uuid => :uuid,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:26Z

+                                              :shared => :shared,
+                                              :sm_config => :sm_config,
+                                              :tags => :tags,
+                                              :type => :type,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:27Z

+                                              :description => :description,
+                                              :introduced_by => :introduced_by,
+                                              :local_cache_enabled => :local_cache_enabled,
+                                              :name => :name,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:27Z

+                                              :current_operations => :current_operations,
+                                              :description => :description,
+                                              :introduced_by => :introduced_by,
+                                              :local_cache_enabled => :local_cache_enabled,


Use the new Ruby 1.9 hash syntax.
Line is too long. [91/80]

houndci-bot · 2017-02-24T05:53:27Z

+                                              :content_type => :content_type,
+                                              :current_operations => :current_operations,
+                                              :description => :description,
+                                              :introduced_by => :introduced_by,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:27Z

+                                              :blobs => :blobs,
+                                              :content_type => :content_type,
+                                              :current_operations => :current_operations,
+                                              :description => :description,


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-02-24T05:53:27Z

+                                              :allowed_operations => :allowed_operations,
+                                              :blobs => :blobs,
+                                              :content_type => :content_type,
+                                              :current_operations => :current_operations,


Use the new Ruby 1.9 hash syntax.
Line is too long. [89/80]

coveralls · 2017-02-24T06:49:24Z

Coverage decreased (-0.07%) to 94.005% when pulling 54a3323 on HashNotAdam:fix/cop into 2a0ba15 on fog:master.

coveralls · 2017-02-24T07:22:44Z

Coverage decreased (-0.07%) to 94.012% when pulling afdcf9d on HashNotAdam:fix/cop into 2a0ba15 on fog:master.

coveralls · 2017-02-24T07:53:19Z

Coverage decreased (-0.07%) to 94.012% when pulling afdcf9d on HashNotAdam:fix/cop into 2a0ba15 on fog:master.

plribeiro3000 · 2017-02-24T12:59:47Z

+
+        private
+
+        def load_data(keep_type = nil, delete_templates = false)


Since this method is for servers collection only maybe you want to rename it to load_servers?

Fair call; I'll trust you.
To be really honest, I only have a simple understanding of what this whole gem does — saw some warnings and thought I could apply some standard code practices 😆
I'll do a little more (hopefully over the next day) and will review this again.

plribeiro3000 · 2017-02-24T13:02:00Z

            @collection_name = collection_name
          end

+          def define_methods(methods)


❤️ 💙 💚 💛 💜

plribeiro3000 · 2017-02-24T13:02:19Z

+            end
+
+            super
+          end


Great refactoring. =)

plribeiro3000 · 2017-02-24T13:03:11Z

+          HOST_METHODS = [
+            :emergency_ha_disable, :list_methods, :local_management_reconfigure,
+            :management_disable, :management_reconfigure, :shutdown_agent
+          ].freeze


Loved this. =)

plribeiro3000 · 2017-02-24T13:03:53Z

+            raise RequestFailed.new(
+              %(#{method}: #{response["ErrorDescription"]})
+            )
          end


Great Great Great Great Great Great Great Great refactoring. =)

plribeiro3000 · 2017-02-24T13:05:14Z

+    pif_class::PIF_METHODS.each do |method|
+      methods.include?(method).must_equal(true)
+    end
+  end


You know when the code is good when the test can be as simple as that. Great refactoring including tests. =)

plribeiro3000 · 2017-02-24T13:07:31Z

@HashNotAdam Thats a great PR. I'm really excited to merge it in.

Could you just fix @houndci-bot comments and the one i made regarding the name of the method before i push this forward please?

HashNotAdam · 2017-02-26T00:10:21Z

NP @plribeiro3000

In reviewing your comments, I've found a bug in load_data which tells me there is no test for the original code so I'll look at that too.

coveralls · 2017-02-26T00:58:11Z

Coverage decreased (-0.1%) to 93.951% when pulling b903c05 on HashNotAdam:fix/cop into 2a0ba15 on fog:master.

HashNotAdam · 2017-02-26T01:34:01Z

@plribeiro3000, when attempting to add a test for the above-mentioned bug, I encountered a few issues.

Firstly, Fog::Compute::XenServer::Real deals with making server connections. I can see there are a few VCR cassettes that exist that were configured for connecting to a local server, however, I was unable to see any documentation regarding how one might start a server for testing. Without a test server, mock, or cassette, I can't be completely certain that there aren't more bugs.

The second issue is related to the first in that I found there is a huge amount of duplication in this class, however, given the lack of tests, I can't safely refactor. As an example, the method I created from existing code (request_get_all_records) is nearly identical to get_records. Also, most of the request files just make a call to @connection.request using the parser Fog::Parsers::XenServer::Base.new.

If you can provide assistance regarding how I could get a suitable test environment or equivalent, I'd be happy to continue refactoring. If not, I'll just clean up what I've already submitted and leave it there.

plribeiro3000 · 2017-03-02T20:43:56Z

@HashNotAdam Sadly there isn't a documentation regarding testing because even the tests were never finished. Those VCR tests were made using a real XenServer installation on a physical server.

Feel free to let this change off for now so we can merge what you already have and later (another PR) we can tackle this. What do you think?

HashNotAdam · 2017-03-02T22:03:37Z

Thanks @plribeiro3000, that sounds good; I'm far too nervous about playing with code I have no way of testing.
If anyone comes up with a way to produce a test environment in the future, I'd be happy jump back in.

plribeiro3000 · 2017-03-02T22:23:19Z

@HashNotAdam Can you confirm nothing broke with those changes?

HashNotAdam · 2017-03-02T22:50:38Z

@plribeiro3000 no, as mentioned above, I know of at least one bug so I need to do a clean up and then I'll let you know when I believe it is good.

TBH, even then, if there are no tests, there is no way to be 100% certain.

plribeiro3000 · 2017-03-03T17:43:13Z

@HashNotAdam Thats ok to not work. hehehhe
We can do a release candidate to be sure. =)

Let me know when we can move forward with this.

HashNotAdam added 17 commits February 24, 2017 11:51

Include Ruby 2.2, 2.3, and 2.4 in Travis tests

9466845

Remove test on Fixnum since it has been removed from Ruby 2.4

58ac795

Include XMLRPC gem if running on Ruby 2.4+

a0a4e39

Replace outdated Rubocop parameter

f3f7d98

Reduce duplication when defining methods

38056d3

Exclude spec directory in Rubocop checks

944ed55

👮 Use double-quoted strings

e9f9239

👮 Remove useless method arguments

f5999d2

👮 Remove useless assignment to a local variable

d489f3d

👮 Ensure there is only one space between a method name and the first …

e6efad4

…argument

👮 Clarify ambiguous splat operator

7f9813b

👮 Replace gsub with tr for simple string comparisons

9396779

👮 Disable test for class length on Fog::Compute::XenServer

eca53b6

👮 Underscore unused block argument

cf0faab

👮 Reduce cyclomatic complexity

25f1feb

👮 Reduce duplication

42d2574

Remove debug code incorrectly commited

54a3323

houndci-bot reviewed Feb 24, 2017

View reviewed changes

Remove %i since it is not supported in Ruby < 2.0

03d8775

👮 Reduce duplication

afdcf9d

plribeiro3000 reviewed Feb 24, 2017

View reviewed changes

HashNotAdam added 3 commits February 26, 2017 11:20

Integrate get_by_reference_or_name_or_uuid into method_missing

73c2911

👮 Use Ruby 1.9 hash syntax

325e077

👮 Make Hound use the Rubocop settings

b903c05


		private

		def load_data(keep_type = nil, delete_templates = false)

+                          end
+                          super
+                        end

Uh oh!

Conversation

HashNotAdam commented Feb 24, 2017

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 24, 2017

Uh oh!

coveralls commented Feb 24, 2017

Uh oh!

coveralls commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plribeiro3000 commented Feb 24, 2017

Uh oh!

HashNotAdam commented Feb 26, 2017

Uh oh!

coveralls commented Feb 26, 2017

Uh oh!

HashNotAdam commented Feb 26, 2017

Uh oh!

plribeiro3000 commented Mar 2, 2017

Uh oh!

HashNotAdam commented Mar 2, 2017

Uh oh!

plribeiro3000 commented Mar 2, 2017

Uh oh!

HashNotAdam commented Mar 2, 2017