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

Defaulting None for optional fields #165

Merged
merged 5 commits into from Aug 16, 2017
Merged

Defaulting None for optional fields #165

merged 5 commits into from Aug 16, 2017

Conversation

@earlbread
Copy link
Contributor

@earlbread earlbread commented Aug 14, 2017

This fixes #70.

@earlbread earlbread force-pushed the earlbread:issue70 branch from fd477bc to 51568fb Aug 15, 2017
Copy link
Member

@dahlia dahlia left a comment

@earlbread Generally it looks good to me. 👍 We have only few things to fix.

int64? price,
bool sale,
uri? url,
);

This comment has been minimized.

@dahlia

dahlia Aug 15, 2017
Member

We should test union tags as well e.g.:

union union-test = tag-a
                 | tag-b (text a, text? b, int64 c, int64 d?);
def test_optional_initializer_test():
product = Product(name=u'coffee', sale=False)
assert product.price is None
assert product.url is None

This comment has been minimized.

@dahlia

dahlia Aug 15, 2017
Member

We should test union tags as well e.g.:

def test_union_tags_optional_initializer():
    t = TagB(a=u'test', c=123)
    assert t.a == u'test'
    assert t.b is None
    assert t.c == 123
    assert t.d is None
@@ -249,3 +249,9 @@ def test_service():
PingService().ping(nonce=u'nonce')
with raises(TypeError):
PingService().ping(wrongkwd=u'a')


def test_optional_initializer_test():

This comment has been minimized.

@dahlia

dahlia Aug 15, 2017
Member

Since the function name already starts with test_ we need no _test suffix here.

@earlbread
Copy link
Contributor Author

@earlbread earlbread commented Aug 16, 2017

@dahlia I fixed things you mentioned. Thank you!

@dahlia
dahlia approved these changes Aug 16, 2017
Copy link
Member

@dahlia dahlia left a comment

Everything looks good to me. 👍

@dahlia dahlia merged commit 97a2e6a into nirum-lang:master Aug 16, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dahlia added a commit to nirum-lang/nirum-python that referenced this pull request Oct 23, 2017
@earlbread earlbread deleted the earlbread:issue70 branch May 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants