Skip to content

Remove duplicated comment #219

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Remove duplicated comment #219

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/fosslight_util/oss_item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,15 @@ def comment(self):
return self._comment

@comment.setter
def comment(self, value):
if not value:
def comment(self, new_comment):
if not new_comment:
self._comment = ""
else:
if self._comment:
self._comment = f"{self._comment} / {value}"
if new_comment.strip() not in self._comment:
Copy link
Contributor

@dd-jy dd-jy Jun 12, 2025

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

@bjk7119 bjk7119 Jun 12, 2025

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

@dd-jy dd-jy Jun 16, 2025

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

@dd-jy dd-jy Jun 24, 2025

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

@bjk7119 bjk7119 Jun 27, 2025

Choose a reason for hiding this comment

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

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

self._comment = f"{self._comment}\n{new_comment}"
else:
self._comment = value
self._comment = new_comment


class FileItem:
Expand Down