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
Contributor

nellaG commented Feb 10, 2018

implements #13 .

@nellaG nellaG self-assigned this Feb 10, 2018

@nellaG nellaG added this to Chosen in Sprint at Seoul in February 2018 via automation Feb 10, 2018

@nellaG nellaG requested review from dahlia and kanghyojun Feb 10, 2018

@dahlia dahlia referenced this pull request Feb 10, 2018

Closed

Optional default tag for union types #13

2 of 2 tasks complete

@dahlia dahlia moved this from Chosen to In progress in Sprint at Seoul in February 2018 Feb 10, 2018

@dahlia dahlia moved this from In progress to In review in Sprint at Seoul in February 2018 Feb 10, 2018

@@ -1098,7 +1107,7 @@ class $className(service_type):
$methodNameMap
])
__nirum_method_annotations__ = $methodAnnotations'

__valerie__ = True

This comment has been minimized.

@nellaG

nellaG Feb 10, 2018

Contributor

oh my

@kanghyojun kanghyojun force-pushed the nellaG:default-tag-for-union branch from 7bd4220 to 02bbdff Feb 11, 2018

@checkmate-bot

This comment has been minimized.

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

This comment has been minimized.

Member

kanghyojun commented Feb 11, 2018

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

@nellaG

This comment has been minimized.

Contributor

nellaG commented Feb 12, 2018

👍

@dahlia

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.

This comment has been minimized.

@dahlia

dahlia Feb 12, 2018

Member

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)

This comment has been minimized.

@dahlia

dahlia Feb 12, 2018

Member

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

This comment has been minimized.

@dahlia

dahlia Feb 12, 2018

Member

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

This comment has been minimized.

@dahlia

dahlia Feb 12, 2018

Member

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 }:

This comment has been minimized.

@dahlia

dahlia Feb 12, 2018

Member

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 nirum-lang:master branch 13 times, most recently from 7d5a611 to 12a5ff2 Feb 15, 2018

@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 nellaG:default-tag-for-union branch from 2e79dd0 to 87d7f5e Feb 28, 2018

@kanghyojun kanghyojun merged commit 9019f2a into nirum-lang:master Feb 28, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dahlia

This comment has been minimized.

Member

dahlia commented Feb 28, 2018

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment