Spec jakartification

[m0mus]

No description provided.

[m0mus]

First draft of jakartified spec is done. Please review.

[jgrassel]

Reviewing. Going to post review comments in small chunks as I review so that they can be addressed with agility.

Page 15: Has a TOC formatting issue where chapter name overlaps page number.
Page 16: Copyright (c) 2019 Eclipse Foundation on page 16 should be ©2020, instead of ©2019, right?
Page 16: Copyright (c) 2018 Eclipse Foundation - should be 2020?
Page 19: Panaki Poddar had left the company several years ago, since then I had taken over the role
Page 21 (doc page 6): Foot note (1) rendering issue? (Is the tooling capable of rendering footnotes the same way as the old tool?)
Page 22 (doc page 7): Terminology note should be in italics (and indented?) Another footnote (2, 3) rendering issue?
Page 23 (doc page 8): Caution section should be in italics (and indented?) Another footnote (4, 5) rendering issue?
Page 25 (doc page 10): Another footnote (6, 7), bullet points for When field-based access is used..., When property-based access is used,..., and Mapping annotations must not be applied...

[jgrassel]

See comments

[jgrassel]

Page 26 (doc page 11): bullet points for When Access(FIELD) is applied to an entity class... and When Access(PROPERTY) is applied..., Another footnote (8) rendering issue? Note that when access types are combined within a class, the Transient annotation should be used to avoid duplicate persistent mappings. should be in italics

Page 27 (doc page 12): bullet points for A simple (i.e., non-composite) primary key..., A composite primary key must correspond..., `A simple primary key or a field or property..., Another footnote (9) rendering issue?

Page 28 (doc page 13): Another footnote (10, 11, 12) rendering issue?

(by footnote rendering issue, I am referring to footnote info being embedded in the referring paragraph, rather than being located at the bottom of the page.)

[m0mus]

@jgrassel I know about it and tried to find a solution. Unfortunately, the version of pdf toolkit we are using renders footnotes in pdf only this way. It may be fixed in the latest pdf toolkit version, because I've seen comments that this change has been committed to master, but I don't know has it been released or not. Anyway, it's not the document issue.

[m0mus]

@jgrassel I am suggesting you reviewing the html version. It has the correct footnotes rendering.

[jgrassel]

@jgrassel I know about it and tried to find a solution. Unfortunately, the version of pdf toolkit we are using renders footnotes in pdf only this way. It may be fixed in the latest pdf toolkit version, because I've seen comments that this change has been committed to master, but I don't know has it been released or not. Anyway, it's not the document issue.

I was wondering, the footnote was one of the reasons I decided to small-chunk my review, in case it was a tool limitation. No sense in further mentioning it in subsequent review comments.

[jgrassel]

PDF page 30/Doc Page 15: Sample query: is left orphaned at the end of the page from its associated content on the next page. Should move it to the top of the next page. Case (a) is not bolded.

PDF page 31/Doc Page 16: Case(b) not bolded. Example 2: looks orphaned from its content, and probably should be at the top of the next page.

PDF page 32/Doc Page 17: Case(a) not bolded.

PDF page 33/Doc Page 18: Case(b) not bolded. Example 3: looks orphaned from its content, and probably should be at the top of the next page.

PDF page 34/Doc Page 19: Case(a) not bolded.

PDF page 35/Doc Page 20: Case(b) not bolded. Example 4: orphaned from its content which starts on the next page.

PDF page 36/Doc Page 21: Case(a), Case(b) not bolded.

PDF page 37/Doc Page 22: Case(a) not bolded, and is orphaned from its content which starts on the next page.

PDF page 38/Doc Page 23: Case(b) not bolded.

PDF page 39/Doc Page 24: Case(a) not bolded, and is orphaned from its content which starts on the next page.

[jgrassel]

PDF page 43/Doc Page 28: bullet points for If the type is a class that is annotated with the Embeddable annotation, If the type of the field or property is one of the following, and The inverse side of a bidirectional relationship

PDF page 44/Doc Page 29: bullet points for The many side of one-to-many / many-to-one bidirectional relationships, For one-to-one bidirectional relationships,, For many-to-many bidirectional relationships either side may be the owning side., If an entity that is the target of the relationship is removed, If the remove operation is applied to a managed source entity,, The mapping of unidirectional one-to-many relationships, and The mapping of unidirectional and bidirectional one-to-one relationships

[jgrassel]

PDF page 45/Doc Page 30: should be italicized: Note that it is the application that bears responsibility for maintaining the consistency of runtime relationships—for example, for insuring that the “one” and the “many” sides of a bidirectional relationship are consistent with one another when the application updates the relationship at runtime. Orphan Example: at the end of the page.

[m0mus]

@jgrassel I investigated the problem with orphaned text and it looks like it's not possible to fix it. Asciidoc doesn't provide any mechanism to say that some blocks should keep together. There are feature requests on their bugs tracker, but as I understand it cannot be solved with pdf converter they use.
Good news is that I've fixed the other problems you reported. I'll wait with PR until you go through the whole spec because it could affect pagination.

[jgrassel]

Noted. I'll omit mentioning orphans as I continue my review.

[jgrassel]

PDF page 50/Doc Page 35:

The JPA 2.2 Spec contains:
Table A contains a foreign key to table B. The foreign key column name is formed as the con- catenation of the following: the name of the relationship property or field of entity A; "_"; the name of the primary key column in table B. The foreign key column has the same type as the primary key of table B.

The JPA 3.0 Spec contains:

• Table A contains a foreign key to table B
   1. The foreign key column name is formed as the concatenation of the following: the name of the relationship property or field of entity A; " _ "; the name of the primary key column in table B. The foreign key column has the same type as the primary key of table B.

[jgrassel]

PDF page 62/Doc Page 47:
The following should be in italics:

Support for the table per concrete class inheritance mapping strategy is optional in this release. Applications that use this mapping strategy will not be portable.
Support for the combination of inheritance strategies within a single entity inheritance hierarchy is not required by this specification.

PDF page 63/Doc Page 48:
Needs bullets:

It provides poor support for polymorphic relationships.
It typically requires that SQL UNION queries (or a separate SQL query per subclass) be issued for queries that are intended to range over the class hierarchy.

Just bringing to attention the difference in footnote rendering here, it clearly references footnote #2, but doesn't print the footnote body. This stresses the need for the asciidoctor tool to better render footnotes (in PDF) in order to keep their use consistent:

For example, assuming the use of an English locale, the following must be passed to the database as undelimited identifers so that they will be treated as equivalent for all databases that comply with the SQL Standard’s requirements for the treatment of “regular identifiers” (undelimited identifiers) and “delimited identifiers” [2]:

[jgrassel]

PDF page 64/Doc Page 49:

This is likely a (mac?) font rendering issue, as it looks like '|' symbols are being used instead of the normal '\' symbol for escaping. I say font rendering because copy/pasting to here shows the character to be the expected \ (edited the following with | symbols to show what it visually looks like on screen to me):

Using annotations, a name is specified as a delimited identifier by enclosing the name within
double quotes, whereby the inner quotes are escaped, e.g., @table(name="|"customer|"") .

[jgrassel]

[jgrassel]

PDF page 67-85/Doc Page 51-70:

The EntityManager API code block has artifacts such as <p>, <code>, basically javadoc instructions which are not going to be processed by Asciidoctor -- should remove these elements if they cannot be processed in a codeblock.

In fact, this part looks like a copy/paste from EntityManager.java rather than a copy from the original PDF, as there is a lot more comments than what is in the JPA 2.2 spec. Do we want to leave all these extra comments in the 3.0 spec doc, or do we want to slim it down to be more like the 2.2 spec doc?

PDF page 78-79/Doc Page 63-64: Missing footnotes [26] and [27] as seen on the JPA 2.2 spec. (Nevermind: I spotted them as Info-attention getters following the end of the code block. I'll leave this comment in here for other reviewers to take note so they won't jump at it either.)

[jgrassel]

PDF page 85/Doc Page 70:
javax.persistence.TransactionRequiredException should be jakarta.persistence.TransactionRequiredException (2x).

PDF page 90/Doc Page 75:
Space in c ascade=ALL

PDF page 93/Doc Page 78:
package javax.persistence; should be package jakarta.persistence;

[jgrassel]

PDF page 97/Doc Page 82:
javax.persistence.lock.scope should be jakarta.persistence.lock.scope (2x)

PDF page 98/Doc Page 83:
javax.persistence.lock.scope should be jakarta.persistence.lock.scope in code block

PDF page 99/Doc Page 84:
Code block has javax.persistence should be jakarta.persistence

PDF page 102/Doc Page 87:
Code block has javax.persistence.lock.scope should be jakarta.persistence.lock.scope
Code block has javax.persistence.lock.timeout should be jakarta.persistence.lock.timeout

PDF page 103/Doc Page 88:
javax.persistence should be jakarta.persistence

[jgrassel]

PDF page 106/Doc Page 91:
Needs a bullet: In general, the lifecycle method of a portable application should not invoke EntityManager or query operations, access other entity instances, or modify relationships within the same persistence context [45: Note that this caution applies also to the actions of objects that might be injected into an entity listener] [46: The semantics of such operations may be standardized in a future release of this specification.]. A lifecycle callback method may modify the non-relationship state of the entity on which it is invoked.

PDF page 114/Doc Page 99:
javax.persistence.validation.mode should be jakarta.persistence.validation.mode (x5)

[jgrassel]

PDF page 115/Doc Page 100:

• javax.persistence.validation.group.pre-persist should be jakarta.persistence.validation.group.pre-persist
• javax.persistence.validation.group.pre-update should be jakarta.persistence.validation.group.pre-updat
• javax.persistence.validation.group.pre-remove should be jakarta.persistence.validation.group.pre-remove
• javax.validation.TraversableResolver should be jakarta.validation.TraversableResolver
• javax.persistence.validation.factory should be jakarta.persistence.validation.factory (x2)

[jgrassel]

PDF page 116/Doc Page 101:

• Formatting markers like <p> in the java code (javadoc)

PDF page 121/Doc Page 106:

• Formatting markers like Map<Class, Subgraph> in the java code (javadoc)

PDF page 126/Doc Page 111:

• javax.persistence.fetchgraph should be jakarta.persistence.fetchgraph (x2)
• javax.persistence.loadgraph should be jakarta.persistence.loadgraph

PDF page 129/Doc Page 114:

• javax.persistence.loadgraph should be jakarta.persistence.loadgraph

[jgrassel]

PDF page 422/Doc Page 407:

• Java EE should be Jakarta EE
• javax.persistence.spi.PersistenceProvider should be `jakarta.persistence.spi.PersistenceProvider
• The following:

acme.jar 
   META-INF/services/javax.persistence.spi.PersistenceProvider 
   com.acme.PersistenceProvider
...

should be:

acme.jar
    META-INF/services/jakarta.persistence.spi.PersistenceProvider 
   com.acme.PersistenceProvider
...

• Change META-INF/services/javax.persistence.spi.PersistenceProvider to META-INF/services/jakarta.persistence.spi.PersistenceProvider in the paragraph following that code block

[jgrassel]

PDF page 423/Doc Page 408:

• javax.persistence.provider should be jakarta.persistence.provider (multiple times)

PDF page 425/Doc Page 410:

• Elements in code block: <p>, <code>getPersistenceProviders</code>, <code>PersistenceProvider</code>

PDF page 426/Doc Page 411:

• Elements in code block: {@link PersistenceProviderResolver}, <code>PersistenceProviderResolver</code>, <code>PersistenceProviderResolver</code>

PDF page 427/Doc Page 412:

• Java Persistence schema generation should probably read Jakarta Persistence schema generation
• Java EE should be Jakarta EE
• javax.persistence.schema-generation.database.action should be jakarta.persistence.schema-generation.database.action

PDF page 428/Doc Page 413:

• javax.persistence.schema-generation.scripts.action should be jakarta.persistence.schema-generation.scripts.action
• javax.persistence.schema-generation.create-source should be jakarta.persistence.schema-generation.create-source
• javax.persistence.schema-generation.drop-source should be jakarta.persistence.schema-generation.drop-source
• javax.persistence.schema-generation.create-database-schemas should be jakarta.persistence.schema-generation.create-database-schemas
• javax.persistence.schema-generation.scripts.create-target, should be jakarta.persistence.schema-generation.scripts.create-target,
• javax.persistence.schema-generation.scripts.drop-target should be jakarta.persistence.schema-generation.scripts.drop-target

PDF page 429/Doc Page 414:

• javax.persistence.database-product-name should be jakarta.persistence.database-product-name
• javax.persistence.database-major-version should be jakarta.persistence.database-major-version
• javax.persistence.database-minor-version should be jakarta.persistence.database-minor-version
• javax.persistence.schema-generation.create-script-source should be jakarta.persistence.schema-generation.create-script-source
• javax.persistence.schema-generation.drop-script-source should be jakarta.persistence.schema-generation.drop-script-source
• javax.persistence.schema-generation.connection should be jakarta.persistence.schema-generation.connection
javax.persistence.sql-load-script-source should be jakarta.persistence.sql-load-script-source

[jgrassel]

PDF page 430/Doc Page 415:

• Java EE should be Jakarta EE
• javax.persistence.sql-load-script-source should be jakarta.persistence.sql-load-script-source
• javax.persistence.validation.factory should be jakarta.persistence.validation.factory
• 9.5.1. javax.persistence.spi.PersistenceProvider should be 9.5.1. jakarta.persistence.spi.PersistenceProvider
• javax.persistence.Persistence should be jakarta.persistence.Persistence

PDF page 431/Doc Page 416:

• javax.persistence.spi.PersistenceProvider should be jakarta.persistence.spi.PersistenceProvider
• Code block contains <p>, {@link Persistence}, {@link EntityManagerFactory}, <code>Persistence</code>, <code>EntityManagerFactory</code>, <code>persistence.xml</code>

[jgrassel]

PDF page 432/Doc Page 417:

• Code block contains: <code>ValidatorFactory</code>, <code>"jakarta.persistence.validation.factory"</code>., <code>"jakarta.persistence.bean.manager"</code>., <p>

PDF page 433/Doc Page 418:

• 9.5.2. javax.persistence.spi.ProviderUtil should be 9.5.2. jakarta.persistence.spi.ProviderUtil
• Code Block contains: {@link PersistenceUtil}, <code>LoadState.LOADED</code>, <p>, <code>FetchType.EAGER</code>

PDF page 434/Doc Page 419:

• Code block contains: <code>LoadState.NOT_LOADED</code>, <p>, <code>LoadState.UNKNOWN</code>, LoadState.LOADED, <code>FetchType.EAGER</code>, <code>LoadState.NOT_LOADED</code>, <code>LoadState.UNKNOWN</code>, <code>isLoadedWithoutReference</code>, <code>FetchType.EAGER</code>, <code>LoadState.LOADED</code>, <code>FetchType.EAGER</code>, <code>LoadState.NOT_LOADED</code>

[jgrassel]

PDF page 435/Doc Page 420:

• Code block contains: <p>, <code>LoadState.UNKNOWN</code>, {@link ProviderUtil}
• 9.6. javax.persistence.spi.PersistenceUnitInfo Interface should be 9.6. jakarta.persistence.spi.PersistenceUnitInfo Interface
• Code block contains {@link EntityManagerFactory}

PDF page 436/Doc Page 421:

• Code block contains: <code>name</code>, <code>persistence.xml</code>, <code>provider</code>, <code>EntityManagerFactory</code>, <code>transaction-type</code>, <code>persistence.xml</code>, <code>jta-data-source</code>, <code>non-jta-data-source</code>

[jgrassel]

PDF page 437/Doc Page 422:

• Code block contains <code>mapping-file</code>, <code>persistence.xml</code>, <code>jar-file</code>, <code>persistence.xml</code>,

PDF page 438/Doc Page 423:

• Code block contains <code>class</code>, <code>persistence.xml</code>, <code>exclude-unlisted-classes</code>, <code>persistence.xml</code>, <code>validation-mode</code>

PDF page 439/Doc Page 424:

• Code block contains <code>property</code>, <code>persistence.xml</code>, {@link PersistenceUnitInfo#getClassLoader}, {@link PersistenceUnitInfo#getNewTempClassLoader}, {@link PersistenceUnitInfo#getClassLoader}, {@link PersistenceProvider#createContainerEntityManagerFactory}

PDF page 440/Doc Page 425:

• javax.persistence.spi.PersistenceUnitTransactionType should be jakarta.persistence.spi.PersistenceUnitTransactionType
• Code block contains {@link EntityManagerFactory}
• javax.persistence.SharedCacheMode should be jakarta.persistence.SharedCacheMode

[jgrassel]

PDF page 441/Doc Page 426:

• Code block contains <code>shared-cache-mode</code>, <code>persistence.xml</code>, {@link PersistenceUnitInfo#getSharedCacheMode()}, <code>Cacheable(true)</code>, <code>Cacheable(false)</code>
• javax.persistence.ValidationMode should be jakarta.persistence.ValidationMode

[jgrassel]

PDF page 442/Doc Page 427:

• 9.6.1. javax.persistence.spi.ClassTransformer Interface should be 9.6.1. jakarta.persistence.spi.ClassTransformer Interface

PDF page 443/Doc Page 428:

• Code block contains: {@link PersistenceUnitInfo#addTransformer PersistenceUnitInfo.addTransformer}

PDF page 444/Doc Page 429:

• 9.7. javax.persistence.Persistence Class should be 9.7. jakarta.persistence.Persistence Class
• Java EE should be Jakarta EE
• Code block contains: <p>, <code>Persistence</code>, {@link * PersistenceUtil PersistenceUtil}

PDF page 445-446/Doc Page 430-431:

• The code block contains code, where the 2.2 spec contains just the signatures (no code in {})

[jgrassel]

PDF page 447/Doc Page 432:

• javax.persistence.lock.timeout should be jakarta.persistence.lock.timeout
• javax.persistence.query.timeout should be jakarta.persistence.query.timeout
• javax.persistence.provider should be jakarta.persistence.provider
• javax.persistence.transactionType should be jakarta.persistence.transactionType
• javax.persistence.jtaDataSource should be jakarta.persistence.jtaDataSource
• javax.persistence.nonJtaDataSource should be jakarta.persistence.nonJtaDataSource
• javax.persistence.sharedCache.mode should be jakarta.persistence.sharedCache.mode
• javax.persistence.validation.mode should be jakarta.persistence.validation.mode
• javax.persistence.validation.group.pre-persist should be jakarta.persistence.validation.group.pre-persist
• javax.persistence.validation.group.pre-update should be jakarta.persistence.validation.group.pre-update
• javax.persistence.validation.group.pre-remove should be jakarta.persistence.validation.group.pre-remove
• javax.persistence.schema-generation.create-script-source should be jakarta.persistence.schema-generation.create-script-source
• javax.persistence.schema-generation.drop-script-source should be jakarta.persistence.schema-generation.drop-script-source

[jgrassel]

PDF page 448/Doc Page 433:

• javax.persistence.sql-load-script-source should be jakarta.persistence.sql-load-script-source
• javax.persistence.schema-generation.database.action should be jakarta.persistence.schema-generation.database.action
• javax.persistence.schema-generation.scripts.action should be jakarta.persistence.schema-generation.scripts.action
• javax.persistence.schema-generation.create-source should be jakarta.persistence.schema-generation.create-source
• javax.persistence.schema-generation.drop-source should be jakarta.persistence.schema-generation.drop-source
• javax.persistence.schema-generation.scripts.create-target should be jakarta.persistence.schema-generation.scripts.create-target
• javax.persistence.schema-generation.scripts.drop-target should be jakarta.persistence.schema-generation.scripts.drop-target
• javax.persistence.jdbc.driver should be jakarta.persistence.jdbc.driver
• javax.persistence.jdbc.url should be jakarta.persistence.jdbc.url
• javax.persistence.jdbc.user should be jakarta.persistence.jdbc.user
• javax.persistence.jdbc.password should be jakarta.persistence.jdbc.password
• javax.persistence.dataSource should be jakarta.persistence.dataSource
• javax.persistence.validation.factory should be jakarta.persistence.validation.factory
• Change the following:

Vendors should use vendor namespaces for properties (e.g., com.acme.persistence.logging). Entries that make use of the namespace javax.persistence and its subnamespaces must not be used for vendor- specific information. The namespace javax.persistence is reserved for use by this specification.

to

Vendors should use vendor namespaces for properties (e.g., com.acme.persistence.logging). Entries that make use of the namespace jakarta.persistence and its subnamespaces must not be used for vendor- specific information. The namespace jakarta.persistence is reserved for use by this specification.

[jgrassel]

PDF page 449/Doc Page 434:

• Javadoc contains: <p>, <code>PersistenceUtil</code>, {@link Persistence}, <code>FetchType.EAGER</code>, <code>isLoaded(Object, String)</code>

[jgrassel]

PDF page 466/Doc Page 451:

• javax.persistence should be jakarta.persistence

PDF page 480/Doc Page 465:

• javax.persistence.sharedCache.mode should be jakarta.persistence.sharedCache.mode

[jgrassel]

PDF page 566/Doc Page 551:

• _ElementCollection_ has _ symbols surrounding it

PDF page 588/Doc Page 573:

• Java Persistence persistence provider should be Jakarta Persistence persistence provider

PDF page 599/Doc Page 584:

• in [a17126] below. should be in 12.2.4.8 below.

PDF page 612/Doc Page 597:

• Various typesetting markers like [a19493]
• Bean validation doesn't need a numeric indentation (ie, 1. 2.0. http://jcp.org/en/jsr/detail?id=380.)

[jgrassel]

persistence_3.0-SNAPSHOT.pdf

I'm providing a copy of the PDF that I built and used for my review, so that you can locate my proofreading comments with your version of the document.

And that concludes my review.

[jgrassel]

@m0mus let me know when you've instrumented my recommended changes and have an updated doc for me to review. Thanks!

[m0mus]

@jgrassel Thanks for your reviews! I didn't have time to work on the doc in the last weeks. I'm planning coming back to this work next week.

[m0mus]

Just bringing to attention the difference in footnote rendering here, it clearly references footnote #2, but doesn't print the footnote body. This stresses the need for the asciidoctor tool to better render footnotes (in PDF) in order to keep their use consistent:

For example, assuming the use of an English locale, the following must be passed to the database as undelimited identifers so that they will be treated as equivalent for all databases that comply with the SQL Standard’s requirements for the treatment of “regular identifiers” (undelimited identifiers) and “delimited identifiers” [2]:

In this case it's not a footnote, but a bibliography reference.

[m0mus]

PDF page 67-85/Doc Page 51-70:

The EntityManager API code block has artifacts such as

, , basically javadoc instructions which are not going to be processed by Asciidoctor -- should remove these elements if they cannot be processed in a codeblock.

In fact, this part looks like a copy/paste from EntityManager.java rather than a copy from the original PDF, as there is a lot more comments than what is in the JPA 2.2 spec. Do we want to leave all these extra comments in the 3.0 spec doc, or do we want to slim it down to be more like the 2.2 spec doc?

I think that it must be the exact copy from the source code.

[m0mus]

PDF page 78-79/Doc Page 63-64: Missing footnotes [26] and [27] as seen on the JPA 2.2 spec. (Nevermind: I spotted them as Info-attention getters following the end of the code block. I'll leave this comment in here for other reviewers to take note so they won't jump at it either.)

Unfortunately, it's not possible to have footnotes in code blocks.

[m0mus]

PDF page 187/Doc Page 172: Is there a way to get bold text in the code blocks depicting the BNF? In the 2.2 spec doc, keywords like FROM, AS, FETCH, TREAT, etc, are in bold.

No

[bshannon]

PDF page 187/Doc Page 172: Is there a way to get bold text in the code blocks depicting the BNF? In the 2.2 spec doc, keywords like FROM, AS, FETCH, TREAT, etc, are in bold.

No

I thought there was a way to specify the language for the code block?

[bshannon]

Wow, @jgrassel, that's a lot of feedback!

I think in most cases it would be faster to correct the problem than it would be to type up a
description of the problem. Maybe @m0mus should commit the document as is and you
can edit it directly to correct the problems and submit your own PR?

Personally, I would not worry about whether the document uses the same font style as the
original spec. Stuff like that we can fix in a later release and doing all that work right now is
a lot of work. I would concentrate on document structure - section headers all marked and at the right level, code bock and tables formatted correctly, figures updates, etc.

The goal right now is to NOT turn this into a "too big to do" project, but rather to find a small
enough amount of work to do that gives us a readable, even if not perfectly formatted,
document. We all want the former (at some point) but if the choice is between the latter
and nothing, I'd choose the latter.

[m0mus]

PDF page 187/Doc Page 172: Is there a way to get bold text in the code blocks depicting the BNF? In the 2.2 spec doc, keywords like FROM, AS, FETCH, TREAT, etc, are in bold.

No

I thought there was a way to specify the language for the code block?

There are no options for BNF.

[lukasj]

@jgrassel I think we should merge this to master as is right now and address your feedback through #268 (or other issue(s)) to move forward

[jgrassel]

@jgrassel I think we should merge this to master as is right now and address your feedback through #268 (or other issue(s)) to move forward

Agreed!