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

Notification Enum Field 수정 #32

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

given131
Copy link
Collaborator

@given131 given131 commented Nov 8, 2022

Summary

#29 에서의 이슈를 해결하였습니다.

  • EnumField 가 아닌, Django 에서의 TextChoices 기능 사용

CheckList

  • Assignee
  • Reviewer
  • Labels
  • Local Test Pass
  • Test Code
  • (Front) Screenshot

Reference

image

Notion Task Link

@given131 given131 self-assigned this Nov 8, 2022
@given131 given131 requested a review from GitHae1337 November 8, 2022 11:50


# TODO - state definition for notification, notification log, and reservation
class EnumNotifcationStatus(Enum):
class EnumNotifcationStatus(models.TextChoices):
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaeyoungleeee
수고하셨습니다!
Python native인 Enum과 어떤 차이가 있는 걸까요?
기존에 구현해주신 Enum을 iterate하면서 직접 field 의 choice 값을 넣어주는 구현해주시는 것도 문제가 없어 보였는데, 왜 바꿔주셨는지 궁금하네요!

Copy link
Collaborator Author

@given131 given131 Nov 8, 2022

Choose a reason for hiding this comment

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

@keetae423

오 좋은 리뷰 감사합니다!

우선 기존에 issue(#29) 로 정리해둔 부분의 근본 원인이 바로, enum field 를 rest_framework 에서 제대로 serialize 해주지 못했던 것으로 확인했습니다.

이를 위해, 애초에 choice 를 넣어둘 때, choice element 의 쌍을 (str, str) 으로 넣어주는 것들도 방법이지만, 이 경우 enum 을 쓰는 것이 무의미해진다고 판단하였습니다. singleton 으로서의 enum 의 기능을 상실하고, 결국 literal 이 되기 때문입니다. serialize 를 해줄 때만 저절로 enum 으로 해주는 선택지들도 있긴 했습니다만 (다른 외부 라이브러리 사용), star 수를 확인했을 때 범용적으로 사용되는 라이브러리는 별로 없더라고요.

다행히 찾던 중 django 에서 defaut 로 enum 의 usecase 와 유사한 상황에서 쓸 수 있는 TextChoices 를 제공하는 것을 확인했습니다. (https://docs.djangoproject.com/en/dev/ref/models/fields/#enumeration-types)
하여, 이 부분을 적용하고자 했습니다!

Copy link
Contributor

@GitHae1337 GitHae1337 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

@GitHae1337 GitHae1337 merged commit a6b9195 into main Nov 8, 2022
@GitHae1337 GitHae1337 deleted the fix/update-notification-serializer branch November 8, 2022 12:00
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.

2 participants