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

Remove multipart-post dependency #13

Open
wants to merge 1 commit into
base: gems-upgrade
Choose a base branch
from

Conversation

nachojorge
Copy link
Collaborator

No description provided.

@nachojorge nachojorge force-pushed the remove-multipart-dependency branch 3 times, most recently from 8168f24 to 0c20d6c Compare February 7, 2025 20:04
@nachojorge nachojorge force-pushed the remove-multipart-dependency branch from 0c20d6c to a07101d Compare February 7, 2025 20:19
Copy link

@gaston-gabadian gaston-gabadian Feb 11, 2025

Choose a reason for hiding this comment

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

Find the minimum possible configuration value to make rubocop pass for the next cops:

  • Metrics/BlockLength
  • Metrics/MethodLength
  • Metrics/CyclomaticComplexity
  • Metrics/AbcSize
  • Metrics/ModuleLength
  • Metrics/PerceivedComplexity
  • Metrics/ParameterLists

Comment on lines 56 to 66
def self.normalize_headers(headers)
normalized = headers.map do |h, v|
if v.is_a?(Array)
[h, v.first]
[h, v.first]
else
[h, v]
[h, v]
end
end

Hash[normalized]
end

Choose a reason for hiding this comment

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

def self.normalize_headers(headers)
  headers.transform_values{|v| v.is_a?(Array) ? v.first : v }
end

Comment on lines 42 to 44
def self.filter_response_headers(headers)
headers.select { |h, v| ALLOWED_RESPONSE_HEADERS.include?(h) }
headers.select { |h, _v| ALLOWED_RESPONSE_HEADERS.include?(h) }
end

Choose a reason for hiding this comment

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

def self.filter_response_headers(headers)
  headers.slice(*ALLOWED_RESPONSE_HEADERS)
end

module Angus
module Unmarshalling

def self.unmarshal_scalar(scalar, type)
return nil if scalar.nil?

case type

Choose a reason for hiding this comment

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

case type
      when :string, :integer, :boolean, :object
        scalar
      when :date
        Date.iso8601(scalar)
      when :date_time
        DateTime.iso8601(scalar)
      when :decimal
        BigDecimal(scalar)
      else
        raise ArgumentError, "Unkonwn type: #{type}"
      end

@@ -0,0 +1,41 @@
Metrics/BlockLength:

Choose a reason for hiding this comment

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

Suggested change
Metrics/BlockLength:
AllCops:
Exclude:
- 'coverage/**/*'
Metrics/BlockLength:

@@ -170,7 +162,7 @@ def self.get_service_definition(code_name, version = nil)
doc_url = self.doc_url(code_name, version)

if doc_url.match('file://(.*)') || doc_url.match('file:///(.*)')

Choose a reason for hiding this comment

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

Suggested change
if doc_url.match('file://(.*)') || doc_url.match('file:///(.*)')
if doc_url.match('file:///?(.*)')

Metrics/AbcSize:
Enabled: false

Style/OptionalBooleanParameter:

Choose a reason for hiding this comment

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

I thinkStyle/OptionalBooleanParameter is a good cop. I would try to fix these warnings or disable them in-line in the code, but not to disable it completely.

Style/OptionalBooleanParameter:
Enabled: false

Lint/RescueException:

Choose a reason for hiding this comment

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

I thinkLint/RescueException is a good cop. I would try to fix these warnings or disable them in-line in the code, but not to disable it completely.

Metrics/ParameterLists:
Enabled: false

Lint/InheritException:

Choose a reason for hiding this comment

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

We need to fix classes inheriting from Exception and keep Lint/InheritException enabled.

request
end

def self.build_multipart_body(params)
@boundary = "----RubyMultipartPost#{SecureRandom.hex}"

Choose a reason for hiding this comment

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

Not fully sure about the use of this instance variable in this module-level method. Can't it's value get modified between this assignment and its later use in the line request['Content-Type'] = "multipart/form-data; boundary=#{@boundary}"?

Comment on lines 16 to +24
@connection = PersistentHTTP.new(
:pool_size => 4,
:pool_timeout => 10,
:warn_timeout => 0.25,
:force_retry => false,
:url => url,

:read_timeout => timeout,
:open_timeout => timeout
pool_size: 4,
pool_timeout: 10,
warn_timeout: 0.25,
force_retry: false,
url: url,

read_timeout: timeout,
open_timeout: timeout
Copy link

Choose a reason for hiding this comment

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

Why do we use these values for the proxy client?

Shouldn't we allow to customize this?

@@ -267,7 +261,6 @@ def self.build_from_variable_fields(variable_fields_hash)
representation_object = representation_class.new(*fields.values, fields.transform_keys(&:to_s))

fields = nil
representation_class = nil
Copy link

Choose a reason for hiding this comment

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

Why did we remove this? Doesn't this keep retained memory?

RE_PATH_PARAM = /:\w+/

SEVERE_STATUS_CODES = %w(500 501 503)
HTTP_METHODS_WITH_BODY = %w[post put].freeze
Copy link

Choose a reason for hiding this comment

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

Also, why are we specifically freezing the strings if the magic comment does it for all strings in this file?

Comment on lines +99 to +102
when '.jpg' then 'image/jpeg'
when '.png' then 'image/png'
when '.gif' then 'image/gif'
else 'application/octet-stream'
Copy link

Choose a reason for hiding this comment

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

What about pdfs or so, if we assign them as octet-streams we dont lose anything in the request?

module Angus
module Remote

VERSION = '0.0.16'
Copy link

Choose a reason for hiding this comment

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

1.0.0 maybe? 👀

def self.unmarshal_scalar(scalar, type)
return nil if scalar.nil?

case type
when :string
#scalar.force_encoding(Encoding::UTF_8)
# scalar.force_encoding(Encoding::UTF_8)
Copy link

Choose a reason for hiding this comment

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

Why don't we delete the comment?

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