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

deserialize tag of union properly #35

Merged
merged 2 commits into from Sep 18, 2016

Conversation

kanghyojun
Copy link
Member

@kanghyojun kanghyojun commented Sep 18, 2016

Fix partially #34

Copy link
Member

@dahlia dahlia left a comment

Choose a reason for hiding this comment

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

일단 제가 코멘트한 부분은 컴파일러 수정은 나중에 따로 하겠다는 결정이라면, 이 PR에서는 고칠 필요가 없는 것이기 때문에, 결정을 확실히 하면 좋을 것 같습니다.

@@ -147,7 +147,7 @@ def deserialize_optional(cls, data):


def deserialize_meta(cls, data):
if hasattr(cls, '__nirum_tag__'):
if hasattr(cls, '__nirum_tag__') or hasattr(cls, 'Tag'):
Copy link
Member

Choose a reason for hiding this comment

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

일단은 이런 방법으로 동작하게 만들 수는 있는데, 좀더 명확하게 알 수 있는 방법이 있으면 좋을 것 같네요. 차라리 컴파일러에서 클래스마다 __nirum_type__ = 'union' 같은 걸 넣어주는 게 나을지도 모르겠네요.

@@ -205,6 +205,18 @@ def deserialize_union_type(cls, value):
raise ValueError('"_type" field is missing.')
if '_tag' not in value:
raise ValueError('"_tag" field is missing.')
if not hasattr(cls, '__nirum_tag__'):
for sub_cls in cls.__subclasses__():
Copy link
Member

Choose a reason for hiding this comment

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

이것도 매번 루프를 돌아야 하니, 수퍼클래스 쪽에 딕셔너리를 만드는 편이 더 낫지 않을까요? 물론 그러려면 컴파일러를 고쳐야 하지만…

Copy link

Choose a reason for hiding this comment

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

그리고 제3자가 임의로 Union에 대한 서브클래스를 만들었을 경우에 대해서도 고려해야 할 것 같습니다.

@kanghyojun kanghyojun merged commit d47479e into nirum-lang:master Sep 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants