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

UIP-2024 Add support for covariant prop / state fields #76

Merged

Conversation

aaronlademann-wf
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf commented Jun 13, 2017

Proposed solution for #49

Ultimate problem:

The over_react transformer did not support the covariant keyword added to the Dart SDK in 1.22.0.

How it was fixed:

The transformer was updated to detect the presence of the keyword, and place it within the setter parameter only.

over_react_covariant-prop-support

Testing suggestions:

  1. Verify that tests pass

Potential areas of regression:

Transformed code


FYA: @Workiva/ui-platform-pp

@aviary-wf
Copy link

Raven

Number of Findings: 0

@@ -328,6 +328,19 @@ main() {
''');
});

test('covariant keyword', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is off here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

facepalm

@jacehensley-wf
Copy link
Contributor

You'll also want to bump the travis.yaml dart SDK version

pubspec.yaml Outdated
platform_detect: "^1.1.1"
quiver: ">=0.21.4 <0.25.0"
dev_dependencies:
browser_detect: "^1.0.4"
matcher: ">=0.11.0 <0.13.0"
coverage: "^0.7.2"
dart_dev: "^1.0.5"
dart_dev: "^1.7.6"
dart_style: ">=1.0.6 <2.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm... must've added it when i was trying to get past the version solver issues w/ analyzer - which ended up being the w_flux package... will remove.

@aaronlademann-wf
Copy link
Contributor Author

@jacehensley-wf I already updated the travis config file dart version.

@aaronlademann-wf aaronlademann-wf force-pushed the UIP-2024_covariant-props branch from 036a2db to 0d87d3e Compare June 13, 2017 17:12
@aaronlademann-wf
Copy link
Contributor Author

@jacehensley-wf @greglittlefield-wf feedback addressed.

@@ -296,7 +296,7 @@ class ImplGenerator {
!member.isSynthetic &&
member.isAbstract &&
member.name.name == name &&
member.returnType?.name?.name == type
member.returnType == type
Copy link
Contributor

Choose a reason for hiding this comment

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

member.returnType is a TypeAnnotation, and type is a String... Not sure how this works.

For better future-proofing, can we change this to the following?

member.returnType?.toSource() == type

@aaronlademann-wf aaronlademann-wf force-pushed the UIP-2024_covariant-props branch from 0d87d3e to 1f7d2f2 Compare June 13, 2017 17:57
@aaronlademann-wf aaronlademann-wf force-pushed the UIP-2024_covariant-props branch from 1f7d2f2 to 329f2b6 Compare June 13, 2017 18:03
@jacehensley-wf
Copy link
Contributor

+1

@codecov-io
Copy link

Codecov Report

Merging #76 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #76      +/-   ##
=========================================
+ Coverage    97.7%   97.7%   +0.01%     
=========================================
  Files          28      28              
  Lines        1387    1389       +2     
=========================================
+ Hits         1355    1357       +2     
  Misses         32      32

@greglittlefield-wf
Copy link
Contributor

QA +10

  • Dev +1's
  • Dev/QA +10
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

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

Successfully merging this pull request may close these issues.

6 participants