Workiva / dart_to_js_script_rewriter

A pub transformer that Rewrites Dart script tags to JavaScript script tags, eliminating 404s and speeding up initial loads. Use when building for deployment.
BSD 3-Clause "New" or "Revised" License
21 stars 20 forks source link

CP-1328 Move dart_to_js_script_rewriter to workiva ownership #28

Closed jayudey-wf closed 8 years ago

jayudey-wf commented 8 years ago

Issue

Source:

Tests:

@trentgrover-wf @maxwellpeterson-wf @evanweible-wf @dustinlessard-wf

codecov-io commented 8 years ago

Current coverage is 100.00%

Branch #28 has no coverage reports uploaded yet.

No diff could be generated. No reports for master found. Review entire Coverage Diff as of cf4d139

Powered by Codecov. Updated on successful CI builds.

trentgrover-wf commented 8 years ago

+1

evanweible-wf commented 8 years ago

+1

dustinlessard-wf commented 8 years ago

+10, pulled down changes and ran:

Dustins-MacBook-Pro:dart_to_js_script_rewriter dustin$ ddev test

::: pub serve --port=0 test
    Loading source assets...
    Loading dart_to_js_script_rewriter and test/pub_serve transformers...
    Serving dart_to_js_script_rewriter test on http://localhost:51442

::: pub run test --concurrency=4 -p vm --reporter=expanded --pub-serve=51442 test/
    00:00 +0: test/dart_to_js_script_rewriter_test.dart: dart_to_js_script_rewriter removeDartDotJsTags
    00:00 +1: test/dart_to_js_script_rewriter_test.dart: dart_to_js_script_rewriter rewriteDartTags
    00:00 +2: test/dart_to_js_script_rewriter_test.dart: dart_to_js_script_rewriter allowedExtensions
    00:00 +3: test/dart_to_js_script_rewriter_test.dart: dart_to_js_script_rewriter apply() when run in release mode
    00:00 +4: test/dart_to_js_script_rewriter_test.dart: dart_to_js_script_rewriter apply() when run in debug mode
    00:00 +5: All tests passed!

::: `pub serve --port=0 test` (buffered stdout)
    Build completed successfully
    [test] GET Served 3 assets.

00:00 +5: All tests passed!
Dustins-MacBook-Pro:dart_to_js_script_rewriter dustin$ ddev analyze

::: dartanalyzer --fatal-warnings example/test.dart lib/dart_to_js_script_rewriter.dart test/dart_to_js_script_rewriter_test.dart test/transformer_mocks.dart tool/dev.dart
    Analyzing [example/test.dart, lib/dart_to_js_script_rewriter.dart, test/dart_to_js_script_rewriter_test.dart, test/transformer_mocks.dart, tool/dev.dart]...
    No issues found
    No issues found
    No issues found
    No issues found
    No issues found

Analysis completed.
jayudey-wf commented 8 years ago

QA Resource Approval: +1

Merging into master.

kasperpeulen commented 8 years ago

@jayudey-wf Hi, I really don't appreciate this.

First you guys, don't respond on my other PR for more than a month. Okay, I can live with that.

Then you open a new PR, that really conflicts with my PR. Okay, well, that is getting problematic of course.

But then you just merge this in, and let me clean up the mess in my PR? Come on...

trentgrover-wf commented 8 years ago

Hi @kasperpeulen

Sorry for the headache. I asked @jayudey-wf to go ahead and merge this PR first because it added the tests and associated infrastructure without making any changes to the transformer code itself. It will be easier to review your PR with confidence with tests already in place.

As to the delay in review: Unfortunately, we clearly weren't watching this repo and have now rectified that issue so it shouldn't happen again.

As to the merge conflicts with your other PR: We made the mess so we'll go ahead and clean it up. We'll put together a PR against your branch that deals with the merge conflict resolution and such. Once you've reviewed and merged that into your branch, we'll be able to review your PR in a clean state.

Again, sorry for trouble.