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

Fixed incorrect call for .map on a hash #51868

Closed
wants to merge 1 commit into from

Conversation

Kaakati
Copy link

@Kaakati Kaakati commented May 20, 2024

Motivation / Background

This Pull Request has been created because There was a mistake in the method implementation where coder.map.key? is called instead of directly using coder.key?("value") The error was causing method 'key?' for #<Enumerator: {}:map> indicates that coder.map returns an enumerator, which does not have a key?

Used a constant for coder args as an empty hash, this is better for optimizing memory usage and ensuring immutability, especially in scenarios where the method might be called frequently without arguments.

Detail

This Pull Request changes:

@value = coder["value"] if coder.map.key?("value")
# with
@value = coder["value"] if coder.key?("value")

set default value for coder argument in each of the following methods:

init_with(coder = EMPTY_HASH)
encode_with(coder = EMPTY_HASH)

Updated the following line to reduce memory allocation.

UNINITIALIZED_ORIGINAL_VALUE = Object.new.freeze

Added a value check in initialize_dup(other)

def initialize_dup(other)
  # Using `defined?(@value)` ensures that it only proceed if `@value` is actually set
  # Ensures no unnecessary memory allocation by checking if `@value` is set and duplicable.
  if defined?(@value) && @value&.duplicable?
    @value = @value.dup
  end
end

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • [*] This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • [*] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • [*] Tests are added or updated if you fix a bug or add a feature.
  • [*] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

There was a mistake in the method implementation where coder.map is called instead of directly using coder.key?("value")
@Kaakati Kaakati marked this pull request as draft May 20, 2024 16:40
@Kaakati Kaakati marked this pull request as ready for review May 20, 2024 16:40
@rafaelfranca
Copy link
Member

Can you please explain why this code was failing, or write a test showing the problem?

@@ -124,15 +127,15 @@ def hash
[self.class, name, value_before_type_cast, type].hash
end

def init_with(coder)
def init_with(coder = EMPTY_HASH)
Copy link
Member

Choose a reason for hiding this comment

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

This method should never be called without an argument, so why this change?

@@ -153,7 +156,9 @@ def original_value_for_database
alias :assigned? :original_attribute

def initialize_dup(other)
if @value&.duplicable?
# Using `defined?(@value)` ensures that it only proceed if `@value` is actually set
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I get his. Why wouldn't @value be set?

@name = coder["name"]
@value_before_type_cast = coder["value_before_type_cast"]
@type = coder["type"]
@original_attribute = coder["original_attribute"]
@value = coder["value"] if coder.map.key?("value")
Copy link
Member

Choose a reason for hiding this comment

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

This mapper can't be an enumerable. It is always an hash. See https://github.com/ruby/psych/blob/master/lib/psych/coder.rb#L14

@rafaelfranca
Copy link
Member

The more I look at those changes, the more I think they are wrong. Perhaps you are miss using init_with and encode_with, that is the only reason why you would not get the right object to those methods.

For example, the YAML::Coder doesn't have a key? method, so this patch can't work. All the tests failing shows that.

@Kaakati Kaakati deleted the coder_patch branch May 22, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants