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

Remove allowing true for annotation traits #381

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

mtdowling
Copy link
Member

This commit removes the support for setting annotation traits to true
because the previous rationale for supporting that no longer holds true.
The previous rationale was that supporting true makes writing the AST
JSON files easier. However, we've already crossed the point where we
don't want humans writing the JSON AST, and we want it to be as easy to
parse as possible.

The AST format now requires that {} is provided for annotation traits.
The IDL no longer supports setting a trait to true or null, as that
was only supported in order to provide isomorphism with the AST support.
We previously didn't persist which model format a pending trait
definition came from, so it was necessary to give them the same feature
set. However, now the Smithy loader maintains a boolean value that tells
the loader whether or not a value is an "annotation", meaning trait with
no value. This isn't a feature used in the AST loader, but is used in
the IDL loader.

Further, the BooleanTrait abstract class was renamed to AnnotationTrait.
This is something I wanted to do for a long time, but never did since it
would cause so much churn, and the name wasn't that far off from what it
represents. However, now that true isn't supported for annotation
traits, the name BooleanTrait makes no sense. This commit renames it and
takes the BC hit because it's now worth it.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mtdowling mtdowling requested a review from kstich April 16, 2020 23:13
@mtdowling mtdowling force-pushed the remove-true-for-annotation-traits branch from 923f1e5 to e15f5ee Compare April 17, 2020 17:17
@mtdowling mtdowling requested a review from kstich April 17, 2020 17:17
This commit removes the support for setting annotation traits to true
because the previous rationale for supporting that no longer holds true.
The previous rationale was that supporting `true` makes writing the AST
JSON files easier. However, we've already crossed the point where we
don't want humans writing the JSON AST, and we want it to be as easy to
parse as possible.

The AST format now requires that `{}` is provided for annotation traits.
The IDL no longer supports setting a trait to `true` or `null`, as that
was only supported in order to provide isomorphism with the AST support.
We previously didn't persist which model format a pending trait
definition came from, so it was necessary to give them the same feature
set. However, now the Smithy loader maintains a boolean value that tells
the loader whether or not a value is an "annotation", meaning trait with
no value. This isn't a feature used in the AST loader, but is used in
the IDL loader.

Further, the BooleanTrait abstract class was renamed to AnnotationTrait.
This is something I wanted to do for a long time, but never did since it
would cause so much churn, and the name wasn't that far off from what it
represents. However, now that `true` isn't supported for annotation
traits, the name BooleanTrait makes no sense. This commit renames it and
takes the BC hit because it's now worth it.
@mtdowling mtdowling force-pushed the remove-true-for-annotation-traits branch from e15f5ee to fc5e2eb Compare April 17, 2020 17:30
@mtdowling mtdowling merged commit c40a385 into 0.10 Apr 17, 2020
@mtdowling mtdowling deleted the remove-true-for-annotation-traits branch April 23, 2020 02:18
@kstich kstich mentioned this pull request Apr 23, 2020
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.

2 participants