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

Add defaults for properties in Records #316

Merged
merged 3 commits into from
Oct 7, 2020

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Oct 6, 2020

Fixes #188.

This small PR adds to #283.

@jhugman jhugman force-pushed the 188-default-properties branch from 8d4678d to c414ed1 Compare October 6, 2020 18:58
{%- match field.default_value() %}
{%- when Some with(literal) %} = {{ literal|literal_py }}
{%- else %}
{%- endmatch -%}
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, this will cause problems in python if there are non-default fields following a default field:

>>> def test(one, two=2, three):
...   pass
  File "<stdin>", line 1
SyntaxError: non-default argument follows default argument>>>                                                       

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yuck. Ok. This was meant to be a quick PR; I think I favour pulling out defaults until we have the bandwidth for #300.

Copy link
Collaborator

@rfk rfk left a comment

Choose a reason for hiding this comment

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

Wasn't sure if this was ready for review, but it seems reasonable to me 👍

I don't mind if we punt on the argument-ordering issue in python, as long as it works sensible in Kotlin and Swift.

@@ -57,6 +57,7 @@ listOf(-1, 0, 1).map { DictionnaireNombresSignes(it.toByte(), it.toShort(), it.t
listOf(0, 1).map { DictionnaireNombres(it.toUByte(), it.toUShort(), it.toUInt(), it.toULong()) }
.affirmAllerRetour(rt::identiqueNombres)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: whitespace

@jhugman jhugman force-pushed the 188-default-properties branch from c414ed1 to 725f16c Compare October 7, 2020 14:02
@jhugman
Copy link
Contributor Author

jhugman commented Oct 7, 2020

I don't mind if we punt on the argument-ordering issue in python

Removed the python implementation in favour of #300.

@jhugman jhugman merged commit 29eebbd into mozilla:main Oct 7, 2020
@jhugman jhugman deleted the 188-default-properties branch October 7, 2020 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out optional dictionary fields
2 participants