Skip to content

Conversation

@bjk7119
Copy link
Contributor

@bjk7119 bjk7119 commented Jun 12, 2025

Description

  • Remove duplicated comment when save comment

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation update
  • Refactoring, Maintenance
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

@bjk7119 bjk7119 requested review from dd-jy and soimkim June 12, 2025 06:43
@bjk7119 bjk7119 self-assigned this Jun 12, 2025
@bjk7119 bjk7119 added the chore [PR/Issue] Refactoring, maintenance the code label Jun 12, 2025
else:
if self._comment:
self._comment = f"{self._comment} / {value}"
if new_comment.strip() not in self._comment:
Copy link
Contributor

Choose a reason for hiding this comment

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

@bjk7119
self._comment를 '/'로 split한 후 not in 체크해야 하는 것 아닌지 확인부탁드립니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 이 부분을 고민했었는데, 아래의 예시로 Test한 결과 Split을 안해도 될 것 같다고 판단했습니다.
즉, 일부만 포함되어도 기존 문자열에 있는 것으로 판단합니다.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@bjk7119
comment에 '/' 라인이 포함되어 있으면 잘못 판단할 수 있을 것으로 보이는데.. 해당 경우에는 split해도 문제가 되긴 하겠네요. comment 중복 추가되는 root 원인 판단하여 수정하고 duplicate 체크는 하지 않는 것이 좋을 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bjk7119
util에서 comment 중복 제거하는 부분은 들어가지 않는 것이 어떨지 의견드립니다. 각 Scanner에서 중복 코멘트가 들어가야 하지 않는다면 Scanner에서 수정되는 것이 맞고, 중복 코멘트가 들어가는 것이 맞는 케이스도 존재할 수 있을 것으로 보입니다.

구분자(\n) 변경 또한 여러줄로 보이면 짧은 코멘트임에도 밑에 줄에 있는 경우 height 조절 전까지 바로 볼 수 없다는 단점이 있고, FL Dependency comment로 direct 패키지 여부 확인 시, '/'로 split하여 확인하고 있기에 Hub 등 많은 곳에서 파생 수정이 필요할 것으로 보입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

동의하여 해당 PR은 Close하겠습니다.

@bjk7119 bjk7119 closed this Jun 27, 2025
@dd-jy dd-jy deleted the fix_dup_cmt branch July 8, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore [PR/Issue] Refactoring, maintenance the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants