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

Fixed nesting bugs for notes and examples #238

Closed
wants to merge 1 commit into from

Conversation

godbyk
Copy link
Contributor

@godbyk godbyk commented Nov 27, 2013

I replaced \enterexample and \exitexample with \begin{example} and \end{example}, respectively. Similarly for \enternote and \exitnote.

By using environments, LaTeX can detect missing \end{...} markers and incorrectly nested environments (i.e., closing the outer environment before closing the inner environment).

The side effects of this change are that any new markup should make the following changes:

  • Change \enterexample to \begin{example}.
  • Change \exitexample to \end{example}.
  • Change \enternote to \begin{note}.
  • Change \exitnote to \end{note}.
  • Change \note to \remark. (Because \note conflicts with \begin{note}. The output is the same as it was previously.)
  • Change \notes to \remarks. (For consistency. The output is the same as it was previously.)

@godbyk
Copy link
Contributor Author

godbyk commented Aug 9, 2014

I've updated this pull request to work with the latest master branch.

@jwakely
Copy link
Member

jwakely commented Aug 11, 2014

I like the change from \notes to \remarks and your rationale for the rest seems convincing (but it's @zygoloid that needs to be convinced.)

Are the only real "nesting bugs" that it fixes the ones in utilities.tex on remove_extent and remove_all_extents?

@godbyk
Copy link
Contributor Author

godbyk commented Aug 11, 2014

Here is a list of nesting errors that I found:

  • overloading.tex:301: swap \end{itemize} and \end{note}
  • utilities.tex:10052: missing \begin{example}
  • utilities.tex:10062: missing \begin{example}
  • locales.tex:247: swap \end{itemize} and \end{note}
  • algorithms.tex:2419: swap \end{itemize} and \end{note}
  • iostreams.tex:858: swap \end{itemize} and \end{note}

@zygoloid
Copy link
Member

I like the idea of this change. I have just a couple of comments on the patch:

+\usepackage{xparse}
+\usepackage{textcase}

It seems like overkill to add extra packages for this; instead, please hardcode the capitalized and uncapitalized forms of note and example.

Can you split the \note(s) -> \remark(s) change out to a separate patch? I'd prefer to not have two notionally independent things done together (even if one depends on the other).

@godbyk
Copy link
Contributor Author

godbyk commented Aug 13, 2014

@zygoloid, I can write the note and example environments out so that they don't use the common underlying block environment. This will obviate the need for the xparse and textcase packages.

I can't, however split the \note\remark change into a separate patch because the \begin{note} macro conflicts with the original \note macro. (Under the covers, \begin{note} gets expanded to \note.) Leaving \note defined as it originally was while using the new \begin{note} environment will cause errors if you try to compile the document.

@zygoloid
Copy link
Member

I can't, however split the \note\remark change into a separate patch because the \begin{note} macro conflicts with the original \note macro.

Can't you make a patch that only does the \note to \remark change, as a prerequisite to this one? Nothing conflicts with \remark, does it?

@godbyk
Copy link
Contributor Author

godbyk commented Aug 14, 2014

@zygoloid, yes, I can do that. Do you want it as a separate pull request or just separate commits?

@zygoloid
Copy link
Member

Separate commits is fine. Thanks!

@godbyk
Copy link
Contributor Author

godbyk commented Aug 14, 2014

@zygoloid, okay, have a look now and see what you think.

@zygoloid
Copy link
Member

Sorry for the delay in getting back to you. I'm very happy with this going ahead; next steps are:

  • rebase the patch onto master
  • fix up any new occurrences that have occurred since the patch was first made
  • one of myself, @jwakely, @burblebee, or @W-E-Brown needs to check that there are no changes here other than the intended ones and merge this change.

Thank you very much for pursuing this!

@godbyk
Copy link
Contributor Author

godbyk commented Sep 24, 2014

@zygoloid, sounds good. I'll rebase and update the patches soon. Thanks!

@godbyk
Copy link
Contributor Author

godbyk commented Sep 26, 2014

@zygoloid: Okay, this should be up to date again. Let me know if there's anything else I can do to help!

CC: @jwakely, @burblebee, @W-E-Brown.

@godbyk
Copy link
Contributor Author

godbyk commented Nov 21, 2014

@zygoloid, @jwakely, @burblebee, @W-E-Brown: I've rebased so it should once again be a clean merge.

@godbyk
Copy link
Contributor Author

godbyk commented Dec 23, 2014

@zygoloid, @jwakely, @burblebee, @W-E-Brown: I've rebased so it should once again be a clean merge.

@tkoeppe
Copy link
Contributor

tkoeppe commented Apr 14, 2015

Ping to the reviewers. If this change is good, then it's worth seeing it through before it drifts away too much. A large change like this is hard to maintain over long periods of time.

@godbyk
Copy link
Contributor Author

godbyk commented Aug 2, 2015

Let me know whenever you decide you'd like to review/merge this pull request and I'll updated it to merge cleanly against HEAD.

@jwakely
Copy link
Member

jwakely commented Nov 16, 2015

If you can update this again I'll review it this week. Thanks.

@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 11, 2015

@godbyk: Hi - could you please take another look?

@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 8, 2016

Since this PR appears to have been abandoned, I pulled out the first of its two commits into the new PR #633.

@godbyk
Copy link
Contributor Author

godbyk commented Mar 9, 2016

I apologize for the delay on this. I didn't notice the comments on the pull request until just now. I'll update this shortly.

@godbyk
Copy link
Contributor Author

godbyk commented Mar 9, 2016

The new note and example environments take an optional argument to override the usual Note and Example text. For example,

\begin{note}
  This is a typical note.
\end{note}

\begin{note}[Note A]
  This note is labeled ``Note A.''
\end{note}

renders as:

[ Note: This is a typical note. — end note ]

[ Note A: This note is labeled “Note A.” — end note ]

@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 21, 2016

@godbyk: This PR doesn't merge cleanly; could you please rebase again?

(Note that the first commit has now already been merged separately by #633.)

@jwakely
Copy link
Member

jwakely commented Mar 21, 2016

I've rebased it on master, the result is at jwakely@734b404

However there is some extra whitespace introduced after the new Note and Example environments are opened and closed, e.g.

diff2

diff1

@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 21, 2016

@jwakely: I think that's acceptable. Having a fully puncutated Note inside an itemizing, running sentence is a typographical travesty as it stands, and space of no space are not going to make it any better. Neither style is worse than the other.

@jwakely
Copy link
Member

jwakely commented Mar 21, 2016

I don't think it's acceptable. I'm certainly not going to merge my branch to master like that, but if @zygoloid wants to that's his call.

The change revealed a number of other bugs though, such as [meta.trans.arr] using [\textit{Example} instead of \enterexample (and not even putting a colon after Example!)

Tables 20 and 55 look better too.

@jwakely jwakely added the big An issue causing a large set of changes, scattered across most of the text. label Mar 21, 2016
@godbyk
Copy link
Contributor Author

godbyk commented Mar 21, 2016

I'll rebase this on master and also look into correcting the spacing issue that @jwakely caught.

@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 21, 2016

@godbyk: maybe hang on just a bit, I've already sent jwakely a fix for his fork of your branch, maybe we don't need to duplicate the work?

@godbyk
Copy link
Contributor Author

godbyk commented Mar 21, 2016

To fix the spacing, I removed the trailing \xspace macros from the note and example environments.

@@ -424,8 +424,8 @@

\pnum
Throughout this International Standard, each example is introduced by
``\enterexample'' and terminated by ``\exitexample''. Each note is
introduced by ``\enternote'' and terminated by ``\exitnote''. Examples
``\begin{example}'' and terminated by ``\end{example}''. Each note is
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a legitimate way to use environments.

(I have already fixed that in a separate branch.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkoeppe I agree. Would you like me to adjust my branch? Is there anything else I can do at this time to assist with this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@godbyk: Thank you, but you can leave it for now; @jwakely has an up-to-date copy of the branch that incorporates some additional adjustments needed to go with this change. Hopefully we'll get it in soon. Thank you very much for preparing this large improvement!

@zygoloid
Copy link
Member

Closed in favor of #759.

@zygoloid zygoloid closed this Jun 22, 2016
@jwakely
Copy link
Member

jwakely commented Jun 22, 2016

Now merged - thanks for doing all the initial work on this.

@godbyk
Copy link
Contributor Author

godbyk commented Jun 22, 2016

@jwakely Thanks for working to get it merged in!

jwakely added a commit to cplusplus/networking-ts that referenced this pull request Oct 5, 2016
Based on changes by Kevin Godby and Thomas Köppe from
cplusplus/draft#238
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
big An issue causing a large set of changes, scattered across most of the text.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants