Add createI18nMixin. #82

mwistrand · 2016-10-18T02:50:35Z

Resolve #81 and #199, and also resolve dojo/i18n#16.

Implementation notes:

createButton is the only component that includes createI18nMixin by default.
If a widget's state.locale is unspecified, the state.locale of each parent is looked up until one is found. Otherwise, i18n.locale is used to determine which messages to load.
If a widget's state.labels is specified, but it has not registered any bundles, then the widget's state is not updated.
The text direction is set via state.rtl. If true, then the widget's node will have a dir="rtl" attribute. If false, the node will have a dir="ltr" attribute. If not a boolean value, then the node will not have a dir attribute at all.
The widget node is also given a data-locale attribute set to the widget's state.locale (if exists). This can facilitate locale-specific styling for any application that may need it.

kitsonk

Also, the code coverage is saying that none of the code is covered. Not sure why.

kitsonk · 2016-10-18T12:10:14Z

src/mixins/createI18nMixin.ts

+/**
+ * A map of bundles registered to each instance.
+ */
+const bundleMap = new WeakMap<I18nMixin<Messages, I18nState>, BundleMap<Messages>>();


Ideally, since the key is the same for all these WeakMaps, it would be better to create an interface for all of them and store all this private data in the same weak map.

kitsonk · 2016-10-18T12:10:48Z

src/mixins/createI18nMixin.ts

+				}
+
+				return bundleLabels;
+			}, {} as I18nLabels);


Possibly use of Object.create(null) here?

kitsonk · 2016-10-18T12:14:58Z

src/mixins/createI18nMixin.ts

+		}
+	}
+}, (instance: I18nMixin<Messages, I18nState>, options?: I18nOptions<I18nState>) => {
+	bundleMap.set(instance, {} as BundleMap<Messages>);


Do you need the coercion here?

kitsonk · 2016-10-18T12:15:59Z

src/mixins/createI18nMixin.ts

+	}
+}, (instance: I18nMixin<Messages, I18nState>, options?: I18nOptions<I18nState>) => {
+	bundleMap.set(instance, {} as BundleMap<Messages>);
+	const bundles = options && options.bundles || instance.bundles;


There is no merging of instance and options bundles? Instance bundles will only be used if there are no options bundles?

My thought here was that bundles would be either registered via instantiation or within mixin/extend, but not both. If you think it would be preferable to merge them, I don't have a problem with that.

I think we should merge them, because you could see a situation where, in a complex widget, where you would have some strings that are coming from the class and some that you might want to configure on the instance, but still have the ones that were baked into the class.

Also, thinking if a mixin has code that is dependent upon a bundle, and then some developer wants to add their own labels, the accidentally break that mixin. Obviously they could "overwrite" the bundle by just using the same key.

kitsonk · 2016-10-18T12:18:54Z

src/mixins/createRenderMixin.ts

@@ -149,6 +151,7 @@ function generateID(cachedRender: RenderMixin<RenderMixinState>): string {
 const widgetClassesMap = new WeakMap<RenderMixin<RenderMixinState>, string[]>();

 const createRenderMixin = createStateful
+	.mixin(createI18nMixin)


I know we sort of talked about this, but this then assumes that EVERY widget will be an i18n widget. Do we want that hard dependency, since it does feel like RenderMixin doesn't have any real dependency on I18N from what I can see.

That's a tough call. On the one hand, mixing it directly into createRenderMixin automatically benefits other out-of-the-box widgets like createButton, so that users don't have to store a separate factory somewhere just to use i18n with those widgets. On the other hand, no, not every widget will need to be localized.

At the very least, I'd argue that the out-of-the-box widgets should include createI18nMixin. Whether they inherit that through createRenderMixin or mix it in themselves depends on whether the additional overhead in createRenderMixin does or does not outweigh the inconvenience of users needing to mix createI18nMixin explicitly into their own widgets.

They would only need to include it if they have some localised text they would need to display. Many widgets, such as a layout panel, or a tabbed bar even, wouldn't have localised strings. I agree, we should include it in any out-of the box widgets where they display a text string of some sort.

mwistrand · 2016-10-18T12:23:00Z

dojo-i18n has not been published to npm yet, so it currently requires a symlink.

codecov-io · 2016-10-18T17:33:07Z

Current coverage is 94.69% (diff: 100%)

Merging #82 into master will increase coverage by 0.39%

@@             master        #82   diff @@
==========================================
  Files            11         12     +1   
  Lines           386        415    +29   
  Methods           6          6          
  Messages          0          0          
  Branches         70         77     +7   
==========================================
+ Hits            364        393    +29   
  Misses           10         10          
  Partials         12         12

Powered by Codecov. Last update 366e435...10b1ac2

mwistrand · 2016-10-18T20:58:08Z

The functional tests that fail here also fail on master (when tested locally).

mwistrand · 2016-10-18T21:04:07Z

Also, dojo/i18n#19 has been resolved, so this issue PR has no outside blockers.

agubler · 2016-12-05T10:19:28Z

tests/unit/mixins/createI18nMixin.ts

+			};
+			return i18n(bundle, 'fr').then(function () {
+				const messages = localized.localizeBundle(bundle);
+				assert.strictEqual(messages['hello'], 'Bonjour');


Can this not be messages.hello?

It can be; I didn't realize TS had been updated to allow dot notation on [key: string] interfaces (previously the threw warnings with dot notation).

agubler · 2016-12-05T10:20:14Z

tests/unit/mixins/createI18nMixin.ts

+			}));
+			localized.localizeBundle(bundle);
+
+			setTimeout(dfd.callback(() => {


let's use dojo-core/timing#delays and return the promise over this.async(...

agubler · 2016-12-05T16:41:11Z

General question, I'm not sure that we should support the ability to pass bundles via options, to me everything i18n should be statically declared within a widget and I cannot think of a reasonable use case for being able to pass in bundles and specify keys via state (get's really messy if they need to be update/configured/modified by actions too)... if you get to that point the author would probably be better to extend a widget and import their nls in the custom widget?

Also do we think we need to support per widget locales? what is the use case to have multiple widgets with differing locales on a page? I do think that there is a use case for support changing the main locale however.

Lastly when the locale changes to how does the new nls bundle get loaded and all the widgets invalidated? (assume this happens internally in the i18n module?)

Sorry for all the questions!

dylans · 2016-12-05T17:12:03Z

Main use case is if someone wanted to build the UI for something like Google translate, or to have an intentionally bilingual UI (common scenario is apps in Canada where the country strongly encourages two main languages, and someone needs to quickly support both languages).

The main use case we've used it for is training purposes, e.g. to show how to use i18n, it helps to be able to show two languages with two instances of the same widget on a page.

agubler · 2016-12-05T17:16:53Z

Main use case is if someone wanted to build the UI for something like Google translate, or to have an intentionally bilingual UI (common scenario is apps in Canada where the country strongly encourages two main languages, and someone needs to quickly support both languages).

Is per widget locale required to support this? Wouldn't this being able to change the main/app locale be sufficient for something like a UI for google translate or a bilingual UI?

mwistrand · 2016-12-05T17:17:49Z

General question, I'm not sure that we should support the ability to pass bundles via options, to me everything i18n should be statically declared within a widget and I cannot think of a reasonable use case for being able to pass in bundles and specify keys via state (get's really messy if they need to be update/configured/modified by actions too)... if you get to that point the author would probably be better to extend a widget and import their nls in the custom widget?

If there is a generic widget that will be reused in a number of contexts (e.g., createSelect that renders a list of options), then forcing users to write something like:

const createCountrySelect = createSelect
    .mixin({
        mixin: { bundles: [ countriesBundle ] }
    });
const countrySelect = createCountrySelect();

Seems more circuitous than it needs to be, with something like the following being preferable:

const countrySelect = createSelect({ bundles: [ countriesBundle ] });

Also do we think we need to support per widget locales? what is the use case to have multiple widgets with differing locales on a page? I do think that there is a use case for support changing the main locale however.

Most applications will not need multiple locales side-by-side, but there are use cases. For example, a language-learning application would have instructions in one locale, but language materials in a different locale. Unless the additional flexibility severely aggravates performance, is there a reason for removing it?

Lastly when the locale changes to how does the new nls bundle get loaded and all the widgets invalidated? (assume this happens internally in the i18n module?)

Right now, dojo-i18n/i18n registers Stateful objects for update when the locale changes. However, dojo-i18n should arguably know nothing about Stateful objects, and as such once dojo/shim#59 lands, dojo-i18n should (imo) be updated to rely on observables.

agubler · 2016-12-05T18:13:33Z

If there is a generic widget that will be reused in a number of contexts (e.g., createSelect that renders a list of options), then forcing users to write something like:

const createCountrySelect = createSelect
.mixin({
mixin: { bundles: [ countriesBundle ] }
});
const countrySelect = createCountrySelect();
Seems more circuitous than it needs to be, with something like the following being preferable:

const countrySelect = createSelect({ bundles: [ countriesBundle ] });

How would createSelect ever know what to do with that bundle or what the keys were without intimate knowledge of the bundle itself (i.e. which would basically need to be done author time). I don't think createSelect would be a generic widget like suggested unless it offered something specific outside of dealing with the nls bundles.

Most applications will not need multiple locales side-by-side, but there are use cases. For example, a language-learning application would have instructions in one locale, but language materials in a different locale. Unless the additional flexibility severely aggravates performance, is there a reason for removing it?

I guess I was just worried about overcomplicating/bloating i18n before we'd got the basics in place for how to integrate into a solution with widgets before extended it to support per widget locale switching.

Right now, dojo-i18n/i18n registers Stateful objects for update when the locale changes. However, dojo-i18n should arguably know nothing about Stateful objects, and as such once dojo/shim#59 lands, dojo-i18n should (imo) be updated to rely on observables.

Does it update the state for all the widgets that are i18n registered? How many renders would this cause in a complex page of internationalized widgets?

mwistrand · 2016-12-05T18:45:12Z

How would createSelect ever know what to do with that bundle or what the keys were without intimate knowledge of the bundle itself

createI18nMixin uses state.labels to map message bundle keys to state properties (and individual widget factories could piggy back off this as needed). Something like createTextInput might be a better example than createSelect:

const input = createTextInput({
    bundles: [ routeSpecificBundle ],
    state: {
        locale: 'fr',
        labels: {
            placeholder: 'routeSpecificPlaceholder'
        }
    }
});

Does it update the state for all the widgets that are i18n registered? How many renders would this cause in a complex page of internationalized widgets?

If the root locale changes, registered widgets are only be invalidated if they don't have their own locale explicitly set. With dojo-shim/Observable, createI18nMixin would subscribe the widget, and then invalidate itself if it does not have its own locale, or if its own state.locale is changed. The locale switch does cascade to children, so there potentially could be a lot of renders especially for complex applications with only a single locale at the root. Then again, dynamic locale switching will happen rarely in the overwhelming majority of applications, and the build will ensure that supported locale data are bundled with the rest of the application, in which case the messages will be available before the next render cycle.

agubler · 2016-12-05T19:46:46Z

createI18nMixin uses state.labels to map message bundle keys to state properties (and individual widget factories could piggy back off this as needed). Something like createTextInput might be a better example than createSelect:
const input = createTextInput({
   bundles: [ routeSpecificBundle ],
   state: {
       locale: 'fr',
       labels: {
           placeholder: 'routeSpecificPlaceholder'
       }
   }
});

For locale's I would envisage that I would always access them in this fashion:

	const messages = this.localizeBundle(textInput);
	return { innerHTML: messages.placeholder };

So if bundles were accepted as an option I would expect the replacement to have the same key as the default bundle within createTextInput and not reference a state attribute placeholder (which at the moment would get set by i18n processing the labels section of the state as I understand it).

Perhaps it would be good to get a demo to show examples of the usage in real widgets set up to go other this as it is pretty hard to completely understand from this alone? What do you think?

mwistrand · 2016-12-05T20:24:09Z

Perhaps it would be good to get a demo to show examples of the usage in real widgets set up to go other this as it is pretty hard to completely understand from this alone? What do you think?

It's outdated, and will be updated again with this implementation, but you can see how this is incorporated into todo-mvc here. As an example, createTodoFooter uses the createButton factory provided out-of-the-box by dojo-widgets to render the "Clear completed" button. Without i18n, the "Clear completed" text is put directly on state.label. With createI18nMixin, the same generic widget is used, but state.labels is used, and state.label is updated with the internationalize "Clear completed" text once the associated bundle has loaded.

localizeBundle could be used to the same effect, but then createButton would have to be extended just for this particular button:

const createClearCompleted = createButton
    .mixin({
        mixin: {
            bundles: [ bundle ],
            nodeAttributes: [
                function (attributes) {
                    const messages = this.localizeBundle(bundle);
                    return { label: messages.clearCompleted };
                }
            }
        }
    });

I'm having a hard time seeing how that is better than the following, which seems to me to be simpler:

const clearCompletedButton = createButton({
    bundles: [ bundle ],
    state: {
        labels: { label: 'clearCompleted' }
    }
});

agubler · 2016-12-05T21:02:42Z

For me in the example of the "clear completed" button in todo-mvc that actual i18n bundle wouldn't be dealt with at the button level. It would be dealt with at the enclosing widgets level, in this case the createTodoFooter and then injected in as state, something like:

import { Widget, WidgetOptions, WidgetState, DNode } from 'dojo-widgets/interfaces';
import createWidgetBase from 'dojo-widgets/createWidgetBase';
import d from 'dojo-widgets/d';
import createButton from 'dojo-widgets/components/button/createButton';
import { clearCompleted } from '../actions/userActions';
import createTodoFilter from './createTodoFilter';
import todoFooterBundle from './nls/todoFooterBundle';

export type TodoFooterState = WidgetState & {
	activeFilter?: string;
	activeCount?: number;
	completedCount?: number;
};

export type TodoFooterOptions = WidgetOptions<TodoFooterState>;

export type TodoFooter = Widget<TodoFooterState>;

const createTodoFooter = createWidgetBase
.mixin(createI18nMixin)
.mixin({
	mixin: {
		tagName: 'footer',
		classes: [ 'footer' ],
                bundle: [ todoFooterBundle ],
		getChildrenNodes: function(this: TodoFooter): (DNode | null)[] {
			const { activeCount, activeFilter, completedCount } = this.state;
                        const messages = this.localizeBundle(todoFooterBundle);
			const countLabel = activeCount === 1 ? messages.itemLeft : messages.itemsLeft;

			return [
				d('span', { 'class': 'todo-count' }, [
					d('strong', [ activeCount + ' ' ]),
					d('span', [ countLabel ])
				]),
				d(createTodoFilter, {
					state: {
						classes: [ 'filters' ],
						activeFilter
					}
				}),
				completedCount ? d(createButton, {
					listeners: {
						click: clearCompleted
					},
					state: {
						label: messages.clearCompleted,
						classes: [ 'clear-completed' ]
					}
				}) : null
			];
		}
	}
});

export default createTodoFooter;

It seems reasonable to assume that generic widgets such as createButton, createTextInput etc etc will actually be managed by an enclosing stateful widget that can specify the bundle required for the application.

agubler · 2016-12-05T21:35:22Z

The same is true if we wanted to implement the createSelect example mentioned earlier, a widget cannot be standalone it has to be at least wrapped in a projector. For the example that uses createSelect and simply displays a list of countries it could look something like this:

createSelect

import { ComposeFactory } from 'dojo-compose/compose';
import d from 'dojo-widgets/d';
import createWidgetBase from 'dojo-widgets/createWidgetBase';
import { DNode, Widget, WidgetState, WidgetOptions } from 'dojo-widgets/interfaces';

type SelectWidgetState = WidgetState & {
     items: string[];
};

type SelectWidget = Widget<SelectWidgetState>;

type SelectWidgetFactory = ComposeFactory<SelectWidget, WidgetOptions<SelectWidgetState>>;

const createSelect: SelectWidgetFactory = createWidgetBase.mixin({
    mixin: {
        tagName: 'ul',
        getChildrenNodes(this: SelectWidget): DNode[] {
            const { state } = this;
            
            return state.items.map((item) =>
                return d('li', [ item ]);
            });
        }
    }
});

export default createSelect;

Main App Widget

import { DNode } from 'dojo-widgets/interfaces';
import createProjector, { Projector } from 'dojo-widgets/createProjector';
import createI18nMixin from 'dojo-widgets/mixins/createI18nMixin';
import d from 'dojo-widgets/d';

import createSelect, { SelectWidget } from './createSelect';
import bundle from 'nls/appBundle';

const createApp = createProjector
.mixin(createI18nMixin)
.mixin({
    mixin: {
        bundle: [ bundle ],
        getNode(this: Projector): DNode {
            const messages = this.localizeBundle(bundle);

            const items = Object.keys(messages).map((key) => {
                return messages[key];
            });

            return d(createSelect, { state: { items } });
        }         
    }
});

export default createApp;

With the final usage being something like:

import createApp from './createApp';

const app = createApp();

app.append().then(() => {
    console.log('application attached');
});

Apologies if there are any typos or syntax errors I wrote this in the comment, I hope it kind of gives an idea of where I am coming from 😄

mwistrand · 2016-12-05T22:40:29Z

Thanks, @agubler. That does clarify your position. This does have the benefit of separating the simplest components like createButton from needing to mix in createI18nMixin. I will simplify the mixin accordingly and push that as soon as possible.

agubler · 2016-12-07T13:02:46Z

@mwistrand this is failing in CI for a lint error.

agubler · 2016-12-07T13:12:17Z

src/mixins/createI18nMixin.ts

+		return locale;
+	}
+
+	const parent = (<any> instance).parent;


we have no reference to the parent any more in widgets on the instance.

agubler · 2016-12-07T13:17:03Z

src/mixins/createI18nMixin.ts

+	/**
+	 * A map of bundles registered to the instance.
+	 */
+	bundles: BundleMap<Messages>;


Can this be a real Map - Map<string, Messages>?

Since we're using localizeBundle instead of state.labels, this is actually no longer needed.

mwistrand · 2016-12-08T16:49:23Z

src/mixins/createI18nMixin.ts

+	dir: string | null;
+}
+
+export type I18nWidget<M extends Messages, S extends I18nState> = I18nMixin<M> & Widget<S>;


Since Widget only requires a single generic, and since Messages is a simple [key: string]: string interface, does is it make sense to specify Messages as another generic for I18nWidget?

agubler · 2016-12-09T15:06:18Z

README.md

+Widgets can be internationalized by mixing in `dojo-widgets/mixins/createI18nMixin`. [Message bundles](https://github.com/dojo/i18n) are added by specifying a `bundles` array to the widget factory prototype. When manually accessing localized messages, the bundle should first be passed to `localizeBundle`. If the bundle supports the widget's current locale, but those locale-specific messages have not yet been loaded, then the default messages are returned and the widget will be invalidated once the locale-specific messages have been loaded. Each widget can have its own locale by setting its `state.locale`; if no locale is set, then the default locale as set by [`dojo-i18n`](https://github.com/dojo/i18n) is assumed.
+
+```typescript
+const createI18nWidget = createWidgetBase


Can we add an example where we use this in getChildrenNodes and pass the i18n values to a child widget via the state?

something like

getChildrenNodes: function() { const messages = this.localizedBundle(greetings); return [ d(createButton, { state: { label: messages.buttonLabel } } ); ]; }

dylans · 2016-12-20T23:59:21Z

@mwistrand This has a conflict because of the change from RxJS to ES8 Observables.

agubler · 2017-01-03T14:09:11Z

package.json

    "dojo-shim": ">=2.0.0-beta.1",
+    "globalize": "^1.1.1",


why is this a peer dependency and not a dependency of i18n?

agubler · 2017-01-03T14:09:50Z

src/mixins/createI18nMixin.ts

+ */
+function getLocaleMessages(instance: I18nWidget<Messages, I18nProperties>, bundle: Bundle<Messages>): Messages | void {
+	const { properties } = instance;
+	const locale = properties['locale'] || i18n.locale;


properties.locale?

agubler · 2017-01-03T14:11:01Z

src/mixins/createI18nMixin.ts

+const createI18nMixin: I18nFactory = compose<I18nMixin<Messages>, WidgetOptions<WidgetState, I18nProperties>>({
+	nodeAttributes: [
+		function (this: I18nWidget<Messages, I18nProperties>, attributes: VNodeProperties): VNodeProperties {
+			const properties = {


lots of properties going on in here, can we call the variable for VNodeProperties vNodeProperties or similar?

agubler · 2017-01-03T14:11:53Z

src/mixins/createI18nMixin.ts

+}, (instance: I18nWidget<Messages, I18nProperties>) => {
+	const subscription = observeLocale({
+		next() {
+			if (!instance.properties['locale']) {


instance.properties.locale

agubler

Just a couple of nits, but otherwise 👍

kitsonk reviewed Oct 18, 2016

View reviewed changes

mwistrand force-pushed the 81-createI18nMixin branch from 5a390da to f495fbd Compare October 21, 2016 16:42

dylans added this to the 2016.10 milestone Oct 24, 2016

mwistrand mentioned this pull request Oct 27, 2016

Incorporate i18n to Monster Cards where possible. dojo/examples#100

Closed

dylans modified the milestones: 2016.11, 2016.10 Oct 31, 2016

mwistrand force-pushed the 81-createI18nMixin branch from 322cac0 to 955901f Compare November 9, 2016 17:26

mwistrand force-pushed the 81-createI18nMixin branch from 1e7825d to ff36008 Compare December 4, 2016 21:07

mwistrand changed the title ~~Add createI18nMixin; mix into createRenderMixin.~~ Add createI18nMixin. Dec 4, 2016

agubler reviewed Dec 5, 2016

View reviewed changes

dylans modified the milestones: 2016.11, 2016.12 Dec 5, 2016

agubler reviewed Dec 7, 2016

View reviewed changes

mwistrand force-pushed the 81-createI18nMixin branch from a6c794e to fc97844 Compare December 8, 2016 12:31

mwistrand commented Dec 8, 2016

View reviewed changes

agubler reviewed Dec 9, 2016

View reviewed changes

mwistrand force-pushed the 81-createI18nMixin branch from b8b80bb to add9a7f Compare December 9, 2016 20:26

dylans modified the milestones: 2017.01, 2016.12 Dec 21, 2016

mwistrand force-pushed the 81-createI18nMixin branch from 2e75596 to 05d3c12 Compare December 28, 2016 23:49

agubler reviewed Jan 3, 2017

View reviewed changes

package.json

"dojo-shim": ">=2.0.0-beta.1",

"globalize": "^1.1.1",

Copy link

Member

agubler Jan 3, 2017

Choose a reason for hiding this comment

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

why is this a peer dependency and not a dependency of i18n?

agubler reviewed Jan 3, 2017

View reviewed changes

agubler approved these changes Jan 3, 2017

View reviewed changes

mwistrand closed this Jan 4, 2017

mwistrand force-pushed the 81-createI18nMixin branch from 10b1ac2 to dbcaf7b Compare January 4, 2017 16:57

This was referenced Jan 4, 2017

WidgetMixin#properties should not be a Partial #199

Closed

LTR and RTL dojo/i18n#16

Closed

mwistrand deleted the 81-createI18nMixin branch February 16, 2017 15:56

Add createI18nMixin. #82

Add createI18nMixin. #82

Conversation

mwistrand commented Oct 18, 2016 • edited Loading

kitsonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwistrand commented Oct 18, 2016

codecov-io commented Oct 18, 2016 • edited Loading

Current coverage is 94.69% (diff: 100%)

mwistrand commented Oct 18, 2016

mwistrand commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler commented Dec 5, 2016

dylans commented Dec 5, 2016

agubler commented Dec 5, 2016

mwistrand commented Dec 5, 2016

agubler commented Dec 5, 2016

mwistrand commented Dec 5, 2016 • edited Loading

agubler commented Dec 5, 2016

mwistrand commented Dec 5, 2016

agubler commented Dec 5, 2016 • edited Loading

agubler commented Dec 5, 2016 • edited Loading

mwistrand commented Dec 5, 2016

agubler commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylans commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler left a comment

Choose a reason for hiding this comment

mwistrand commented Oct 18, 2016 •

edited

Loading

codecov-io commented Oct 18, 2016 •

edited

Loading

mwistrand commented Dec 5, 2016 •

edited

Loading

agubler commented Dec 5, 2016 •

edited

Loading

agubler commented Dec 5, 2016 •

edited

Loading