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

AF-3085: Transformer is aware of Dart 2 builder compatible boilerplate for Factories #205

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

sebastianmalysa-wf
Copy link
Contributor

@sebastianmalysa-wf sebastianmalysa-wf commented Nov 29, 2018

Ultimate problem:

The Dart 2 builder compatible boilerplate for factories includes an initialization statement for a UiFactory, e.g.:

@Factory()
- UiFactory<FooProps> Foo;
+ // ignore: undefined_identifier
+ UiFactory<FooProps> Foo = $Foo;

The problem is that the transformer throws an error if a UiFactory declaration is initialized.

How it was fixed:

  • Make the transformer aware that Dart 2 builder compatible boiler plate initializes the UiFactory so it doesn't throw an error when initialized correctly
  • Ensure (by adding a test) that the transformer will replace the initialization with:
    ([Map backingProps]) => new _$FooPropsImpl(backingProps);

Testing suggestions:

CI passes
Code changes and added tests make sense


FYA: @aaronlademann-wf @corwinsheahan-wf @evanweible-wf @greglittlefield-wf

…tion is initialized with $<UiFactory> for Dart 2 builder compatibility and will replace the initialization with the Map BackingProps as it does currently for Dart 1
@sebastianmalysa-wf sebastianmalysa-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Nov 29, 2018
@sebastianmalysa-wf sebastianmalysa-wf requested a review from a team as a code owner November 29, 2018 19:13
@aviary-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@codecov-io
Copy link

codecov-io commented Nov 29, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   94.58%   94.59%   +0.01%     
==========================================
  Files          34       34              
  Lines        1660     1661       +1     
==========================================
+ Hits         1570     1571       +1     
  Misses         90       90

logger.error(
'Factory variables are stubs for the generated factories, and should not have initializers.',
'Factory variables are stubs for the generated factories, and should not have initializers '
'unless initialized with \$<UiFactory> for Dart 2 builder compatibility.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can have a more helpful error message here. UiFactory is a type so I don't think it should be used as a placeholder here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe just include the expected string?

'unless initialized with \$$factoryName...';

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

This looks good to me. But i'd defer to @greglittlefield-wf for a final signoff.

@greglittlefield-wf greglittlefield-wf self-requested a review December 3, 2018 19:42
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

+1

@evanweible-wf
Copy link
Contributor

QA +1

  • CI passes

  • All WSD tests passed on Dart 1 using this branch

    diff --git a/pubspec.yaml b/pubspec.yaml
    index 0f69468f3..b0d5cd041 100644
    --- a/pubspec.yaml
    +++ b/pubspec.yaml
     environment:
       sdk: ">=1.24.2 <2.0.0"
    +dependency_overrides:
    +  over_react:
    +    git:
    +      url: git@github.com:sebastianmalysa-wf/over_react.git
    +      ref: AF-3085
     dependencies:
       barback: ">=0.15.2 <=0.15.2+14"
       browser: ">=0.10.0 <0.11.0"
    17:51 +27294: All tests passed!
    

@Workiva/release-management-p

@rmconsole4-wk rmconsole4-wk merged commit 42ead71 into Workiva:master Dec 3, 2018
@evanweible-wf evanweible-wf added this to the Dart 2 Compat milestone Dec 21, 2018
@evanweible-wf evanweible-wf modified the milestones: Dart 2 Compat, 2.0.0 (Dart 1 & 2 compatibility) Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility Merge Requirements Met RM Ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants