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

Optional default tag for union type #227

Merged
merged 14 commits into from
Feb 28, 2018

Conversation

nellaG
Copy link
Contributor

@nellaG nellaG commented Feb 10, 2018

implements #13 .

@nellaG nellaG added typ:enhance Type: Enhancement/new feature cmp:compiler Component: Compiler backend (e.g., annotation processors, code generators) target:python cat:lang Category: Language design labels Feb 10, 2018
@nellaG nellaG self-assigned this Feb 10, 2018
@nellaG nellaG requested review from dahlia and kanghyojun February 10, 2018 13:26
@dahlia dahlia mentioned this pull request Feb 10, 2018
2 tasks
@@ -1098,7 +1107,7 @@ class $className(service_type):
$methodNameMap
])
__nirum_method_annotations__ = $methodAnnotations'

__valerie__ = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my

@kanghyojun kanghyojun force-pushed the default-tag-for-union branch from 7bd4220 to 02bbdff Compare February 11, 2018 11:31
@checkmate-bot
Copy link

checkmate-bot commented Feb 11, 2018

Checklist 🤔

src/Nirum/Parser.hs

  • If a new reserved keyword is introduced, it has to be also added to reservedKeywords set in the Nirum.Constructs.Identifier module.

@kanghyojun
Copy link
Member

I changed template engine of union type on Python target as well. cc @nellaG @dahlia

@nellaG
Copy link
Contributor Author

nellaG commented Feb 12, 2018

👍

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.

Need changelog as well.

tag = do
annotationSet' <- annotationSet <?> "union tag annotations"
spaces
-- CHECK: If a new reserved keyword is introduced, it has to be also
-- added to `reservedKeywords` set in the `Nirum.Constructs.Identifier`
-- module.
Copy link
Member

Choose a reason for hiding this comment

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

This check comment should go to the top of Parser.hs source. See also Checkmate docs. Since it's sensitive to block indentation levels, if a CHECK comment is too deeply inside it could be hardly warned.

@@ -1104,36 +1121,33 @@ class $className({T.intercalate "," $ compileExtendClasses annotations}):
else:
name = attribute_name
tag_types = cls.__nirum_tag_types__
if callable(tag_types): # old compiler could generate non-callable map
# old compiler could generate non-callable map
if callable(tag_types):
tag_types = dict(tag_types())
try:
args[name] = deserialize_meta(tag_types[name], item)
Copy link
Member

Choose a reason for hiding this comment

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

Despite this kind of conditionals are necessary for the runtime library to be compatible old and new versions of the compiler, it's no more needed for compiler-generated codes; it's guaranteed that it's always generated by the new one.

@@ -200,6 +207,9 @@ runCodeGen :: CodeGen a
-> (Either CompileError' a, CodeGenContext)
runCodeGen = C.runCodeGen

renderCompileText :: BI.Markup -> T.Text
renderCompileText = toStrict . renderMarkup
Copy link
Member

@dahlia dahlia Feb 12, 2018

Choose a reason for hiding this comment

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

These toStrict $ renderMarkup ... codes have remained by intention; they need to be merely removed in the near future by replacing type CompileResult Python = Code with type CompileResult Python = BI.Markup.

@@ -1007,17 +1018,10 @@ class $className(object):
]
compileTypeDeclaration src
d@TypeDeclaration { typename = typename'
, type' = UnionType tags
, type' = u
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to define tags like the following code in where clause:

tags :: [Tag]
tags = findTags u

Instead we can just match both of UnionType and its tags:

, type' = u@(UnionType tags)

Also we should rename u to better one.

{arg "cls" "type"}, value
){ ret className }:
#{arg "cls" "type"}, value
)#{ ret className }:
Copy link
Member

Choose a reason for hiding this comment

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

Since we now have proper conditionals through Heterocephalus, things like arg "cls" "type" or ret className can be avoided. See also compileTypeDeclaration function for EnumType:

    @classmethod
%{ case pyVer }
%{ of Python2 }
    def __nirum_deserialize__(cls, value):
%{ of Python3 }
    def __nirum_deserialize__(cls: type, value: str) -> '#{className}':
%{ endcase }
        return cls(value.replace('-', '_'))  # FIXME: validate input

@dahlia dahlia force-pushed the master branch 13 times, most recently from 7d5a611 to 12a5ff2 Compare February 18, 2018 10:28
@dahlia dahlia added this to the Version 0.4.0 milestone Feb 18, 2018
kanghyojun added a commit to kanghyojun/nirum-1 that referenced this pull request Feb 20, 2018
@kanghyojun kanghyojun force-pushed the default-tag-for-union branch from 2e79dd0 to 87d7f5e Compare February 28, 2018 13:44
dahlia
dahlia previously approved these changes Feb 28, 2018
@kanghyojun kanghyojun merged commit 9019f2a into nirum-lang:master Feb 28, 2018
@dahlia
Copy link
Member

dahlia commented Feb 28, 2018

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:lang Category: Language design cmp:compiler Component: Compiler backend (e.g., annotation processors, code generators) target:python typ:enhance Type: Enhancement/new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants