Framework: Use a simple JS object to declare the attribute's source by youknowriad · Pull Request #2854 · WordPress/gutenberg

youknowriad · 2017-10-02T12:24:45Z

Description

This PR can be seen as the first step towards #2751

Instead of using the hpq matchers, we use a declarative API to define the attributes source

//before
import { attr } from 'api';

attributes: {
  url: {
    type: 'string',
    source: attr( 'src' ),
  }
}

// after
attributes: {
  url: {
    type: 'string',
    source: 'attribute',
    attribute: 'src',
  }
}

Caveats

A bit verbose compared to the previous approach, noticeable when nesting

// Gallery

images: {
  type: 'array',
  source: 'query',
  selector: 'div.wp-block-gallery figure.blocks-gallery-image img',
  query: {
    source: 'object',
    object: {
      url: {
        type: 'attribute',
        attribute: 'src',
      },
      alt: {
        type: 'attribute',
        attribute: 'alt',
      },
      id: {
        type: 'attribute',
        attribute: 'data-id',
      },
    },
  },
},

Pros

Permits server-side attribute declaration (which opens the way to server-side attribute parsing if necessary)
No hpq direct dependency, no need to wrap the matchers

Checklist:

westonruter · 2017-10-02T16:49:05Z

If verbosity is a concern then in JS there could be shorthand “macros” like:

const attr = ( key ) => ( {
    type: 'attribute',
    attribute: key
} );

In that way you could still do:

attributes: {
  url: {
    type: 'string',
    source: attr( 'src' ),
  }
}

If you wanted to.

aduth · 2017-10-02T17:20:13Z

Talking with @mtias about this, it was mentioned maybe we don't need to nest source, and rather create the source-relevant properties on the same top-level, i.e.

// Before:
attributes: {
  url: {
    type: 'string',
    source: {
      type: 'attribute',
      attribute: 'src',
    }
  }
}

// After:
attributes: {
  url: {
    type: 'string',
    source: 'attribute',
    attribute: 'src',
  }
}

youknowriad · 2017-10-03T12:14:02Z

@aduth How would you declare query( 'div', query( 'p', attr( 'className' ) ) )?

Nesting is needed IMO, and I personally prefer an explicit source property to declare the way we parse an attribute.

ellatrix · 2017-10-04T12:14:18Z

source: 'attribute',
attribute: 'src',

😵

aduth · 2017-10-04T14:29:30Z

I may be playing devil's advocate here, since I'd originally been a proponent of the nested source. But I'm trying to be sensitive to the impact of nesting, which I think can be harmful to general readability of the block definition and, from the perspective of the implementer, how obvious it is to structure. JSON schema is easy to pick on here, because it's... excessive ([1], [2]). While the requirements forced us to rethink the structure, I long for the simplicity we once had for defining attributes:

gutenberg/editor/blocks/image/index.js

Lines 11 to 15 in 376d25d

    
           attributes: { 
        
           	url: attr( 'img', 'src' ), 
        
           	alt: attr( 'img', 'alt' ), 
        
           	caption: html( 'figcaption' ) 
        
           },

(Compare to current)

Of course, where we balance is on consistency and accommodating the varied needs of defining the source, where a nested object could have some merit.

@aduth How would you declare query( 'div', query( 'p', attr( 'className' ) ) )?

If we consider selector and source as the respective arguments to query:

{
	type: 'query',
	selector: 'div',
	source: {
		type: 'query',
		selector: 'p',
		source: {
			type: 'attribute',
			attribute: 'class',
		},
	},
}

Also worth considering optimizing for the more common scenario: query is the edge case, and has shown to have few use-cases thus far. Could even call into question its overall value and whether it's worth keeping around.

source: 'attribute',
attribute: 'src',

😵

You are going to have to elaborate here 😃 I assume you're not a fan of the redundancy? type may not be strictly necessary if we could infer the source type by the presence of other properties. But there is some value in being able to quickly determine (both visually for the reader, and programmatically) the type.

What would you see as the alternative here? type: 'attribute', name: 'src' ? Would it be as obvious the connection between the name property and the type of the source?

ellatrix · 2017-10-04T14:32:30Z

You are going to have to elaborate here 😃

I have no critique, I just thought it was funny. :)

youknowriad · 2017-10-04T16:06:38Z

@aduth so for the first level, the source type is stored in "source", and the nested levels it's in "type". I don't like this because it's not consistent.

Overall what you propose is just considering the first level as a superset of "source".

aduth · 2017-10-09T15:06:51Z

The intent would be to optimize for the common use-case, which is not query. In retrospect, I can grant that it's not great for source to be either an object or a string. To a previous point, maybe the string version of source is redundant if it can be inferred from other properties. Or maybe we need to take a harder look at the use cases for query and whether they can be solved other ways (nested blocks?) so that it can be eliminated.

aduth · 2017-10-13T11:54:22Z

Actually, the non-nested query example can be written without introducing inconsistency to how source is used:

{
	type: 'array',
	source: 'query',
	selector: 'div',
	query: {
		source: 'query',
		selector: 'p',
		query: {
			source: 'attribute',
			attribute: 'class',
		},
	},
}

youknowriad · 2017-10-13T11:57:05Z

@aduth Oh! nice proposal, I'll try to update accordingly.

aduth · 2017-10-13T12:12:48Z

Just now realizing I had type and source mixed up in both of my examples; didn't mean for there to be nearly as much inconsistency as there appears to be.

youknowriad · 2017-10-13T13:35:42Z

Updated per suggestion

aduth · 2017-10-13T14:18:48Z

blocks/library/gallery/index.js

+			source: 'query',
+			selector: 'div.wp-block-gallery figure.blocks-gallery-image img',
+			query: {
+				source: 'object',


Do we need this 'object' source at all? Is it entirely specific to the query behavior?

We need something to distinguish query( 'div', html() ) and query( 'div', { a: html(), b: prop() } and it's not easy to avoid ambiquity between an object defining thee shape of the sub sources or an object defining a regular source.

Hmm, I see the issue. I don't think we want to natively support the concept of sub-sources, but rather enable specific sources like query to encapsulate the behavior they need. In other words, I'd really prefer we not have object be a proper source. Will poke at this a bit, since it's not obvious what a solution would be.

Thinking about this more, I think this matcher can make sense even without query if you want to shape an attribute as an object composed from several sub matchers.

Anyway, I'm open to suggestions here.

In that case, it still doesn't seem to follow that the source is an object (at least this is how it reads when seeing source: 'object'), but rather the desired shape is an object of a particular structure. The feature itself is not one I think we need to support at this point either.

(Will take a closer look shortly for more concrete suggestions)

honestly, I still think the current approach is the best approach (consistent and unambiguous)

Personally I find the object source to be very ambiguous when considered not just within the context of the query source.

The examples I'm seeing where we'd need to change the structure for a flattened query array are: pullquote, quote, and text columns, correct? And because of how they interact with Editable? Are these not the same types of blocks we'd be porting to use proper block nesting? Wondering if we're optimizing for a problem that won't exist.

Correct, these are the concerned blocks. I didn't consider quotes as container for paragraph blocks but I guess you're right, it's a good fit.

So what's the "temporary" option we should go with right now? removing the non-object query and transforming the value in edit/save for these blocks?

So what's the "temporary" option we should go with right now? removing the non-object query and transforming the value in edit/save for these blocks?

I'd probably be okay with this, yes. Motivation to get progress on nested blocks 😄

updated in a4c2982

Yes, quotes would be containers for paragraph blocks, and potentially support a few others, like lists, etc.

aduth · 2017-10-13T14:20:03Z

blocks/api/parser.js

+	}
+}
+
+/**


Description here doesn't seem accurate to the arguments accepted (specifically block type).

aduth · 2017-10-13T14:21:26Z

blocks/api/parser.js

+ * @param  {Object} attributeSchema     Attribute's schema
+ * @param  {string} rawContent          Block's raw content
+ * @param  {Object} commentAttributes   Block's comment attributes
+ *


From what I can tell, there is no mixed type for JSDoc, but rather the * (any) type:

http://usejsdoc.org/tags-param.html#multiple-types-and-repeatable-parameters

aduth · 2017-10-13T14:23:02Z

blocks/api/serializer.js


 		// Ignore values sources from content and post meta
-		if ( attributeSchema.source || attributeSchema.meta ) {
+		if ( attributeSchema.source !== 'comment' ) {


Will this change cause meta attributes to be serialized?

no this function should only take care of comment attributes, so better be explicit about it.

aduth · 2017-10-13T14:24:30Z

blocks/api/parser.js

+	let value;
+	switch ( attributeSchema.source ) {
+		case 'meta':
+			break;


Hmm, we've tried to avoid being explicit about comment as a source, treating it as a default. Here instead we treat default as though source is one of the hpq matchers. Maybe instead we should include the cases for those sources in matcherFromSource and let comment be the default case?

Hmm, we've tried to avoid being explicit about comment as a source

Why? I see value in leaving the matchers as the non-explicit ones to avoid listing all the matchers.

It makes sense to me to have a clear matcherFromSource function responsible only for sourcing attributes from the content.

Why? I see value in leaving the matchers as the non-explicit ones to avoid listing all the matchers.

Implementation aside, it's if we conceptually consider serialized comments to be the default storage for attributes, but support other strategies for sourcing these values.

To this point, we ought to consider consistency with how we refer to the commentAttributes argument here. In createBlockWithFallback, this is named attributes. In parseWithGrammar, the block node property is named attrs.

Do you think I should use attrs in all these places?

I'm not a huge fan of abbreviations. If we were to think of comment attributes as the "base" attribute set, then attributes would be appropriate.

it's confusing to me. The function is called getBlockAttributes, it shouldn't have attributes as an argument don't you think?

Yes, this is confusing, I agree. This ties into disjointedness of our attribute sourcing generally (https://github.com/WordPress/gutenberg/pull/2854/files#r144836758). What are these attributes which are magically prepopulated? Are the comments a core part of what a block is, or an incidental implementation detail? In the current context, I could see parsedAttributes making sense, but our approach of passing content and comment attributes as arguments doesn't really scale well to other source types, and as implemented we are aware of all but implementing behaviors of only some types.

How capable do we want the parsing to be? I don't necessarily think we need to build in concepts of post meta and site options to the parser itself, but maybe as some extensibility pattern: either allowing additional sources to inject their values to be referenced by the parser when encountering that source type, or hooks to allow the post editor a turn when encountering the parsing or serialization behaviors of a specific source type.

Alternatively, if it's the case that our editor selector is responsible for managing the implementation of specific types (post meta), why should this not also extend to sourcing from content as well? Ultimately maybe there is a line where a blocks core knows to handle serialized and markup-sourced attributes, then allow additional handlers to be reflected later. But ideally not with built-in awareness of what those other handlers are; if we dropped case 'meta' and treated comments as the default, would it work just as well?

if we dropped case 'meta' and treated comments as the default, would it work just as well?

Can you provide a code example, I don't understand what you're proposing here.

: either allowing additional sources to inject their values to be referenced by the parser when encountering that source type,

This won't work for site options and post meta because their value change over time, and using the parser for this is not a good option. The parser is meant for parsing post_content (HTML) and the content contains only two types of attributes: comment attributes and attributes parsed from HTML.

hooks to allow the post editor a turn when encountering the parsing or serialization behaviors of a specific source type.

Maybe, this means hooks to the getBlock selcetor and updateAttributes callback.

I think we're struggling here because we're trying to fix the symptoms instead of trying to fix the root issue. And the root issue is the Editor not being generic enough. Things would much clearer if the Editor was a generic content editor (not tied to post or site) with extensibility mechanisms.

I do think this is out of scope of the current PR

I think we're struggling here because we're trying to fix the symptoms instead of trying to fix the root issue. And the root issue is the Editor not being generic enough. Things would much clearer if the Editor was a generic content editor (not tied to post or site) with extensibility mechanisms.

This is really what I'm trying to get at in my earlier comment, and arguably it is in scope because we're introducing knowledge of a meta type to the parser with these changes that had not existed previously.

aduth · 2017-10-13T14:24:53Z

blocks/api/registration.js

+
+	const attributes = settings.attributes
+		? settings.attributes
+		: get( window._wpBlocksAttributes, name, );


Drop trailing comma on arguments.

aduth · 2017-10-13T14:27:05Z

editor/modes/visual-editor/block.js


 		const metaAttributes = reduce( attributes, ( result, value, key ) => {
-			if ( type && has( type, [ 'attributes', key, 'meta' ] ) ) {
+			if ( type && get( type, [ 'attributes', key, 'source' ] ) === 'meta' ) {


Minor unrelated note: type && is redundant here (get will handle the falsey type)

aduth · 2017-10-13T14:29:47Z

There are failing tests in block registration.

youknowriad · 2017-10-13T16:11:22Z

There are failing tests in block registration.

Yep, I didn't take care of tests and documentation yet, wanted to leave this after we settle on the format. I can do so now.

codecov · 2017-10-16T09:06:10Z

Codecov Report

Merging #2854 into master will decrease coverage by 0.24%.
The diff coverage is 71.79%.

@@            Coverage Diff             @@
##           master    #2854      +/-   ##
==========================================
- Coverage   34.61%   34.36%   -0.25%     
==========================================
  Files         261      261              
  Lines        6769     7007     +238     
  Branches     1231     1317      +86     
==========================================
+ Hits         2343     2408      +65     
- Misses       3733     3837     +104     
- Partials      693      762      +69

Impacted Files	Coverage Δ
blocks/library/button/index.js	`9.3% <ø> (-1.81%)`	⬇️
blocks/library/code/index.js	`50% <ø> (-7.15%)`	⬇️
blocks/library/image/index.js	`50% <ø> (-2.39%)`	⬇️
blocks/library/shortcode/index.js	`40% <ø> (-10%)`	⬇️
blocks/library/html/index.js	`16.66% <ø> (-6.42%)`	⬇️
blocks/library/cover-image/index.js	`30.43% <ø> (-2.9%)`	⬇️
blocks/library/paragraph/index.js	`29.16% <ø> (-2.84%)`	⬇️
blocks/library/list/index.js	`6.89% <ø> (-1.06%)`	⬇️
blocks/hooks/anchor.js	`80% <ø> (ø)`	⬆️
blocks/library/preformatted/index.js	`44.44% <ø> (-5.56%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b56f09c...2aeb51a. Read the comment docs.

youknowriad · 2017-10-16T09:16:07Z

Updated this PR with unit tests and documentation, it's in a better shape now.

aduth · 2017-10-16T12:13:05Z

Hit another merge conflict 😄

aduth · 2017-10-16T12:26:58Z

blocks/api/parser.js

-		result[ key ] = coercedValue;
-		return result;
-	}, {} );
+	const blockAttributes = keys( blockType.attributes ).reduce( ( memo, attributeKey ) => {


Lodash's reduce could make this a little more concise:

const blockAttributes = reduce( blockType.attributes, ( memo, attributeSchema, attributeKey ) => {

aduth · 2017-10-16T12:28:17Z

blocks/api/parser.js

+		const attributeSchema = blockType.attributes[ attributeKey ];
+		memo[ attributeKey ] = getBlockAttribute( attributeKey, attributeSchema, rawContent, attributes );
+		return memo;
+	}, {} ) || {};


I don't think the || {} would ever be triggered? Even if the reduce operates on an empty set, the initial value should be returned.

> [].reduce( () => {}, {} ); {}

aduth · 2017-10-16T12:41:00Z

blocks/api/registration.js

-		attributes: get( window._wpBlocksAttributes, name ),
 		...settings,
+		name,
+		attributes: keys( attributes ).reduce( ( memo, attributeKey ) => {


From your previous comment:

I see value in leaving the matchers as the non-explicit ones to avoid listing all the matchers.

Is the whole existence of this reduce here to support being able to specify the comment case?

aduth · 2017-10-16T12:50:03Z

blocks/api/parser.js

+ */
+export function getBlockAttribute( attributeKey, attributeSchema, rawContent, commentAttributes ) {
+	let value;
+	switch ( attributeSchema.source ) {


Our approach for defining other attribute sources is a bit awkward, I think, in that we have to explicitly bypass the getter here to allow it to be handled elsewhere later. Would like if these strategies were more consolidated. cc @mcsf

aduth · 2017-10-16T12:52:15Z

blocks/api/parser.js

+	let value;
+	switch ( attributeSchema.source ) {
+		case 'meta':
+			break;


To this point, we ought to consider consistency with how we refer to the commentAttributes argument here. In createBlockWithFallback, this is named attributes. In parseWithGrammar, the block node property is named attrs.

aduth · 2017-10-16T12:55:56Z

blocks/library/gallery/index.js

+			source: 'query',
+			selector: 'div.wp-block-gallery figure.blocks-gallery-image img',
+			query: {
+				source: 'object',


A few ideas:

We could support one or the other, but not both, where query can either return an array of objects shaped of a particular structure, or an array of a particular source value

We could split the two options by property name, e.g. queryObject (array of sourced object shape) and query

We could test the presence of query.source and infer if it's defining a shape or the nested source

youknowriad · 2017-11-13T09:46:20Z

A final review for this?

aduth

Not sure if it's introduced by this pull request or an issue that's been resolved since last rebase, but I'm seeing many invalid blocks when simply saving Demo and refreshing the page.

aduth · 2017-11-15T16:10:45Z

blocks/api/parser.js


 	// If the block supports a custom className parse it
-	if ( blockType.className !== false && attributes && attributes.className ) {
+	if ( false !== blockType.className && attributes && attributes.className ) {


Thinking you might have had to make these changes only because you were running an older version of the branch with newer dependencies? (Related: #3187)

Noting that this received no response. It is not a blocker to me for merge.

aduth · 2017-11-15T16:12:09Z

blocks/api/serializer.js

@@ -102,7 +102,7 @@ export function getCommentAttributes( allAttributes, blockType ) {
 		}

 		// Ignore values sources from content and post meta


We should update this comment.

…st API changes

Rebased this and addressed the comments. I'm not seeing the "invalid" issue, it might have been fixed by the rebase.

aduth · 2017-11-15T17:03:08Z

blocks/api/registration.js

This assignment seems unnecessary given let block = blocks[ name ] = { above.

aduth · 2017-11-15T21:12:59Z

blocks/api/registration.js

Was the change to extract this out necessary? Is it just for providing the default to get, and if so, could that have been added in-place?

No reason, just a rebase leftover

BoardJames · 2017-11-21T03:47:37Z

blocks/api/index.js

-
-export { source };
 export { createBlock, switchToBlockType, createReusableBlock } from './factory';
 export { default as parse, getSourcedAttributes } from './parser';


Bug here: getSourcedAttributes was removed from parser

See PR with fix here:
#3582

youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress labels Oct 2, 2017

youknowriad self-assigned this Oct 2, 2017

youknowriad requested a review from aduth October 2, 2017 12:24

youknowriad force-pushed the try/declarative-attributese branch from 40d3d40 to f9c9356 Compare October 13, 2017 13:33

aduth reviewed Oct 13, 2017

View reviewed changes

youknowriad force-pushed the try/declarative-attributese branch from 0a1b070 to ad04011 Compare October 16, 2017 09:13

youknowriad removed the [Status] In Progress Tracking issues with work in progress label Oct 16, 2017

aduth reviewed Oct 16, 2017

View reviewed changes

youknowriad force-pushed the try/declarative-attributese branch from ad04011 to 18c5e05 Compare October 17, 2017 09:22

aduth mentioned this pull request Oct 25, 2017

Editable: Treat value as HTML string, eliminate children source #2750

Closed

aduth mentioned this pull request Nov 2, 2017

Parser: Allow block nesting #2743

Merged

youknowriad force-pushed the try/declarative-attributese branch from f0aa05f to 456f054 Compare November 10, 2017 08:48

aduth previously requested changes Nov 15, 2017

View reviewed changes

youknowriad added 12 commits November 15, 2017 17:46

Framework: Use a simple JS object to declare the attribute's source

1a6817e

Block API: Simplify the source declaration (drop one nesting level)

4598bb2

Blocks API: Fix some styling issues per review

c55bde6

Tests: Fix existing unit tests after attributs refactoring

cc4b4fd

Tests: Add getBlockAttribute unit test

d82ceab

Documentation: Update the attributes' documentation to match the late…

f213476

…st API changes

Blocks API: Simplify logic using lodash's reduce

07c0bad

Parser: Only support query with object nesting

7c300a2

Parser: Avoid explicit handling of non-content attributes

2873c65

Parser: Treat attributes with "undefined" source as "comment" attributes

254a351

Remove duplicate docs and keep property matcher undocumented

a6ffcaa

Parser: Fix small code style per review

032ddc4

youknowriad force-pushed the try/declarative-attributese branch from 104a32a to 207d794 Compare November 15, 2017 16:55

aduth reviewed Nov 15, 2017

View reviewed changes

aduth approved these changes Nov 15, 2017

View reviewed changes

youknowriad force-pushed the try/declarative-attributese branch from 207d794 to c013766 Compare November 16, 2017 07:46

Extensibility: Fix the anchor extension after rebase

2aeb51a

youknowriad force-pushed the try/declarative-attributese branch from c013766 to 2aeb51a Compare November 16, 2017 07:49

youknowriad merged commit 9cf1e9e into master Nov 16, 2017

youknowriad deleted the try/declarative-attributese branch November 16, 2017 07:55

youknowriad mentioned this pull request Nov 16, 2017

Block API: Ensure backwards compatibility for block matchers #3519

Merged

aduth mentioned this pull request Nov 21, 2017

Block API: Server-side awareness of block types #2751

Closed

BoardJames reviewed Nov 21, 2017

View reviewed changes

BoardJames mentioned this pull request Nov 21, 2017

fix/2854: getSourcedAttributes was not completely removed #3582

Merged

3 tasks

aduth mentioned this pull request Nov 27, 2017

Documentation: Remove "source" key when using meta attributes #3652

Closed

youknowriad mentioned this pull request Feb 26, 2018

Simplify quote values #5265

Closed

3 tasks

+              	}
+              }
+              /**

		@@ -102,7 +102,7 @@ export function getCommentAttributes( allAttributes, blockType ) {
		}

		// Ignore values sources from content and post meta

Conversation

youknowriad commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Caveats

Pros

Checklist:

Uh oh!

westonruter commented Oct 2, 2017

Uh oh!

aduth commented Oct 2, 2017

Uh oh!

youknowriad commented Oct 3, 2017

Uh oh!

ellatrix commented Oct 4, 2017

Uh oh!

aduth commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellatrix commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Oct 4, 2017

Uh oh!

aduth commented Oct 9, 2017

Uh oh!

aduth commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Oct 13, 2017

Uh oh!

aduth commented Oct 13, 2017

Uh oh!

youknowriad commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

youknowriad commented Oct 2, 2017 •

edited

Loading

aduth commented Oct 4, 2017 •

edited

Loading

ellatrix commented Oct 4, 2017 •

edited

Loading

aduth commented Oct 13, 2017 •

edited

Loading

codecov bot commented Oct 16, 2017 •

edited

Loading