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

Convert mutable type which is unhasable to immutable types #123

Merged
merged 11 commits into from Jun 1, 2017

Conversation

kanghyojun
Copy link
Member

it fixes nirum-lang/nirum-python#49. this PR propose convert a type that could be mutable to immutable type always rather than raise error on runtime.

@kanghyojun kanghyojun added typ:bug Type: Bug/defect typ:enhance Type: Enhancement/new feature labels Mar 19, 2017
@kanghyojun kanghyojun self-assigned this Mar 19, 2017
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.

함수 이름에 대한 댓글을 많이 달았습니다. toX와 같은 이름들이 쓰였는데, 하스켈의 X로 만든다는 느낌이 먼저 들었기 때문입니다. 실제로는 파이썬의 값을 파이썬의 X로 만드는 파이썬 코드를 생성해주는 함수였기 때문에 다른 이름들을 제안했습니다.

attributeValue :: T.Text
attributeValue = case fieldType' of
SetModifier _ -> [qq|frozenset($attributeName)|]
ListModifier _ -> [qq|tuple($attributeName)|]
Copy link
Member

Choose a reason for hiding this comment

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

리스트를 튜플로 표현하는 것이 적절하지는 않은 것 같습니다. 튜플을 불변 리스트로 보는 사람이 별로 없기 때문입니다. 차라리 런타임에 ImmutableList를 만들어서 넣고 그걸 쓰는 건 어떨까요?

fieldName' :: Name
fieldName' = fieldName field
fieldType' :: TypeExpression
fieldType' = TD.fieldType field
Copy link
Member

Choose a reason for hiding this comment

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

fieldName', fieldType' 이 둘은 getter 쓰는 것보다 데이터 생성자 써서 패턴 매칭으로 구하는 게 더 깔끔하지 않을까요?

@@ -329,6 +339,28 @@ returnCompiler = do
Python2 -> ""
Python3 -> [qq| -> $r|]

convertFieldToImmutable :: Field -> Code
convertFieldToImmutable field =
Copy link
Member

Choose a reason for hiding this comment

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

compileFieldInitializer 정도의 이름을 제안합니다.

_ -> attributeName

toClassInitialValues :: [Field] -> [Code]
toClassInitialValues fields =
Copy link
Member

Choose a reason for hiding this comment

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

비슷하게 compileClassFieldInitializers 정도의 이름을 제안합니다.

toArgumentCode :: (ParameterName -> ParameterType -> Code)
-> [(T.Text, Code)]
-> Code
toArgumentCode gen nameNTypes = toIndentedCodes (uncurry gen) nameNTypes ", "
Copy link
Member

Choose a reason for hiding this comment

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

Parameter는 보통 형식 인자(인자의 이름이나 타입과 같은 모양)을 뜻하고 argument는 그 인자에 들어가는 실제 인자값을 뜻합니다. 예를 들어 def f(a, b): 같은 함수를 정의할 때 ab는 parameters지만 그걸 호출하는 f(1, 2)에서 1이나 2는 arguments입니다.

그런 차원에서 생각하면 함수 이름은 argument라는 말보다는 parameter라는 말을 쓰는 게 나을 것 같네요.

compileParameters 정도의 이름을 제안합니다.

toArgumentCode gen nameNTypes = toIndentedCodes (uncurry gen) nameNTypes ", "

toInitialValueCode :: DS.DeclarationSet Field -> Code
toInitialValueCode fields =
Copy link
Member

Choose a reason for hiding this comment

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

compileFieldInitializers 정도의 이름을 제안합니다. 그리고 이하의 convertFieldToImmutable, toClassInitialValues 같은 함수들은 단위 테스트를 붙이기 위한 것이 아니라면 차라리 이 함수의 where 절 아래로 넣는 게 낫지 않을까요?

@kanghyojun
Copy link
Member Author

@dahlia ImmutableList로 갈아치우기위해 nirum-lang/nirum-python#65 를 올렸습니다. 물론 ImmutableList를 사용하기 위해선 [qq|ImmutableList(...)|]이런 식으로 바로 되는 것은 아니고, 런타임을 추가하기위해 thirdPartyImports를 호출해야합니다. 일단 런타임 머지, PyPI 배포 후에 이 PR은 다시 진행합니다.

case fieldType' of
SetModifier _ -> return [qq|frozenset($attributeName)|]
ListModifier _ -> do
let imports = [("nirum.datastructures", ["List"])]
Copy link
Member

Choose a reason for hiding this comment

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

It prevents Nirum types to be named as list since it can make name conflict. Seems nirum.datastructures need to export aliases e.g. list_type, map_type, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

i need to modify nirum-python first, then i'll fix this!

Copy link
Member Author

Choose a reason for hiding this comment

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

nirum-lang/nirum-python#70 nirum-python fixed. From now on, List aliased as list_type.

@kanghyojun
Copy link
Member Author

🙇🏼 Kore o kakuninshitekudasai 🙇🏼

album = Album(name='25', tracks=[
Song(name='Hello')
])
assert isinstance(album.tracks, tuple)
Copy link
Member

Choose a reason for hiding this comment

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

tuple이 아니라 nirum.datastructures.List인지 검사해야 할 것 같습니다.

- Update minimum required version of nirum
- Unicode lietral have to be used on Python2.7
Importing "List" would raise error, because "List" could be named on
user-define variable. so list_type which is alias for List is imported.
@kanghyojun kanghyojun merged commit 9e24232 into nirum-lang:master Jun 1, 2017
@dahlia dahlia added cmp:compiler Component: Compiler backend (e.g., annotation processors, code generators) target:python labels Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmp:compiler Component: Compiler backend (e.g., annotation processors, code generators) target:python typ:bug Type: Bug/defect typ:enhance Type: Enhancement/new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants