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

CP-1328 Move dart_to_js_script_rewriter to workiva ownership #28

Merged
merged 2 commits into from
Feb 4, 2016

Conversation

jayudey-wf
Copy link
Contributor

Issue

  • the dart_to_js_script_rewriter was transferred to Workiva and hadn't been updated to include Workiva information or to follow some of the Workiva repo patterns

Changes

Source:

  • added three tests to allow for 100% coverage
  • updated readme with appropriate badges
  • updated example directory to reference an index.html page instead of test.html
  • utilized dart_dev

Tests:

  • added these tests: allowedExtensions and the apply() tests

Areas of Regression

  • n/a (no source code change)

Testing

  • passing CI

Code Review

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

@codecov-io
Copy link

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.

..pubServe = true
..unitTests = ['test/'];

config.coverage..pubServe = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

cascade is unnecessary here if we're only setting one thing

@jayudey-wf jayudey-wf force-pushed the updates branch 2 times, most recently from 60c6205 to 2f41241 Compare February 2, 2016 19:07
@@ -71,5 +75,7 @@ See the [pub docs][pubdocs] for more on modes.

Please use the [issue tracker][issues].

[issues]: https://github.com/sethladd/dart_to_js_script_rewriter/issues
[issues]: https://github.com/workiva/dart_to_js_script_rewriter/issues

Choose a reason for hiding this comment

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

workiva -> Workiva

@@ -1,5 +1,9 @@
# dart_to_js_script_rewriter

[![Pub](https://img.shields.io/pub/v/dart_to_js_script_rewriter.svg)](https://pub.dartlang.org/packages/dart_to_js_script_rewriter)
[![Build Status](https://travis-ci.org/Workiva/dart_to_js_script_rewriter.svg?branch=travis-ci)](https://travis-ci.org/Workiva/dart_to_js_script_rewriter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing badges like this, give problems with dartdoc. The link won't work in pub.dartlang.org. Doing it with pure html, will make it work in github as well on pub.

Choose a reason for hiding this comment

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

link to docs is already available on pub in the right side of the screen
screen shot 2016-02-03 at 11 47 54 am

Choose a reason for hiding this comment

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

nm, I see your point

@trentgrover-wf
Copy link

+1

1 similar comment
@evanweible-wf
Copy link
Contributor

+1

@dustinlessard-wf
Copy link

+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 jayudey-wf changed the title Move dart_to_js_script_rewriter to workiva ownership CP-1328 Move dart_to_js_script_rewriter to workiva ownership Feb 4, 2016
@jayudey-wf
Copy link
Contributor Author

QA Resource Approval: +1

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • performed and documented by Dustin Lessard
  • Unit test created/updated
  • All unit tests pass

Merging into master.

jayudey-wf added a commit that referenced this pull request Feb 4, 2016
CP-1328 Move dart_to_js_script_rewriter to workiva ownership
@jayudey-wf jayudey-wf merged commit 72943dd into master Feb 4, 2016
@kasperpeulen
Copy link
Contributor

@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
Copy link

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.

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.

6 participants