serialized_property setter의 불필요한 dirty marking 수정 #23
Conversation
…marking when values remain unchanged. Add comprehensive test coverage for edge cases.
Summary of ChangesHello @robin-watcha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
serialized_property setter에서 동일한 값을 할당할 때 불필요하게 dirty로 마킹되던 문제를 수정한 좋은 변경입니다. 이전 값과 새 값을 비교하여 실제로 변경이 있을 때만 _will_change!를 호출하도록 수정한 접근 방식이 올바릅니다.
_write_sprop_value와 _define_serialized_property 두 곳에 변경 사항을 일관되게 적용한 점이 좋습니다. 또한, 다양한 시나리오(같은 값, 다른 값, nil 처리, setter proc 등)를 포괄하는 종합적인 테스트 케이스를 추가하여 수정된 내용의 안정성을 크게 높인 점이 인상적입니다.
한 가지 개선 제안으로, 두 메서드에 중복되는 로직이 있어 이를 별도의 헬퍼 메서드로 추출하여 코드 유지보수성을 높이는 방안을 리뷰 의견으로 남겼습니다. 전반적으로 훌륭한 수정입니다!
| hsh = send(col) | ||
| if value.nil? | ||
| old_value = hsh[key.to_s] | ||
|
|
||
| new_value = if value.nil? | ||
| nil | ||
| else | ||
| apply_setter && setting[:setter] ? setting[:setter].call(value) : value | ||
| end | ||
|
|
||
| # Skip dirty marking if value hasn't changed | ||
| return old_value if old_value == new_value | ||
|
|
||
| send("#{col}_will_change!") | ||
|
|
||
| if new_value.nil? | ||
| hsh.delete(key.to_s) | ||
| else | ||
| value = setting[:setter].call(value) if apply_setter && setting[:setter] | ||
| hsh[key.to_s] = value | ||
| hsh[key.to_s] = new_value | ||
| end | ||
| value | ||
| new_value |
There was a problem hiding this comment.
이 로직은 _define_serialized_property 메서드 내에 정의된 setter (326-345행)의 로직과 거의 동일하여 코드 중복이 발생합니다. 유지보수성을 높이기 위해 이 공통 로직을 별도의 private 헬퍼 메서드로 추출하는 것을 고려해 보세요.
예를 들어, 다음과 같은 헬퍼 메서드를 if defined?(ActiveRecord::Base) && base < ActiveRecord::Base 블록 내에 정의할 수 있습니다.
private
def _update_sprop_if_changed(column, key, hsh, old_value, new_value)
return old_value if old_value == new_value
send("#{column}_will_change!")
if new_value.nil?
hsh.delete(key.to_s)
else
hsh[key.to_s] = new_value
end
new_value
end그런 다음 _write_sprop_value와 _define_serialized_property의 setter에서 이 헬퍼 메서드를 호출하여 중복을 제거할 수 있습니다. 이렇게 하면 코드가 더 간결해지고 향후 수정이 필요할 때 한 곳만 변경하면 됩니다.
요약
serialized_propertysetter에서 동일한 값으로 설정 시 불필요하게 dirty로 마킹되는 버그 수정_will_change!호출문제 현상
수정 내용
_write_sprop_value메서드:write_attribute,[]=호출 시 값 비교 로직 추가_define_serialized_propertysetter: 일반 setter 호출 시 값 비교 로직 추가테스트
write_attribute,[]=접근자 테스트🤖 Generated with Claude Code