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

Issue 36 editorial issues #42

Merged
merged 7 commits into from
Mar 13, 2019
Merged

Issue 36 editorial issues #42

merged 7 commits into from
Mar 13, 2019

Conversation

gkellogg
Copy link
Member

@gkellogg gkellogg commented Mar 1, 2019

  • Remove true and false as values of @embed.
  • Switch sentence order for Implicit Inclusion Flag.
  • Add issue Remove @omitDefault? #41 marker for removing @omitDefault.

Preview | Diff

@gkellogg gkellogg requested a review from iherman March 1, 2019 22:27
Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

In example 13 the caption refers to @omitDefault, but it is not present in the frame itself. It should unless its default value is true. However, if the latter is true, then this should be explicitly said in the preceding paragraph. (I realize there is a pending issue on this, though.)

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

This all looks ok to me other than my one comment.

index.html Outdated
@@ -1147,7 +1207,7 @@ <h3>Framing Algorithm</h3>

<p>The recursive algorithm operates with a <a>framing state</a> (<var>state</var>),
created initially using
the <a>object embed flag</a> set to <code>true</code>,
the <a>object embed flag</a> set to <code>@always</code>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is a change from the current algorithm. true mapped to @last. Is this intentional? If so, it will make the default behavior of framing potentially add data to the output -- which is something we avoided in the past.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps it should be @last? The Boolean values were holdovers and I don’t think they make as much sense going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be @last to keep the previous behavior. To be clear, I'm fine with removing the boolean values -- I just wanted to make sure we weren't making an unintentional change here. If we change this to @last it should keep the same behavior as before. If @first becomes a thing and provides significant performance improvements, we should consider switching this to @first.

gkellogg added a commit that referenced this pull request Mar 10, 2019
This was referenced Mar 10, 2019
@gkellogg gkellogg merged commit 8f754cc into master Mar 13, 2019
@gkellogg gkellogg deleted the issue-36-editorial-issues branch March 13, 2019 00:48
gkellogg added a commit that referenced this pull request Mar 13, 2019
@gkellogg gkellogg mentioned this pull request Mar 13, 2019
5 tasks
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.

None yet

3 participants