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

Ignore # characters inside double quotes and backticks #658

Merged
merged 8 commits into from
Oct 2, 2019

Conversation

Minoru
Copy link
Member

@Minoru Minoru commented Sep 28, 2019

When reviewing #584, I didn't consider that a pound character can appear inside titles or regexes, not just in front of a comment. This introduced a regression, which was reported in #652 when 2.17 was released.

This PR makes utils::strip_comments aware of double quotes and backticks, so it now ignores pound signs inside.

I'd be really happy if someone reviewed this within 20 hours, so I can make a release on Sunday. Otherwise I'll wait my ordinary three days, and release this on Thursday.

@Minoru Minoru force-pushed the bugfix/652-quoted-pound-sign-treated-as-comment branch from e52bdc7 to 6252386 Compare September 28, 2019 20:52
@der-lyse
Copy link
Contributor

This looks very good to me. The only thing I can think of is that we could add some tests which mix both double quotes as well as backticks and nest them in different ways. Looking at the code these conditions are correctly handled, but the tests do not verify them.

@Minoru
Copy link
Member Author

Minoru commented Sep 28, 2019

@noctux pointed out that I never reset prev_was_backslash on characters other than # and `. I force-pushed a fix.

@Minoru
Copy link
Member Author

Minoru commented Sep 28, 2019

@der-lyse, good suggestion; will do!

@Minoru
Copy link
Member Author

Minoru commented Sep 28, 2019

Another one from @noctux over on IRC: how should \# be handled? His expectation was for it to not start a comment; I think it should start one. Previously, comments were handled by utils::tokenize_quoted. There is no test for this function that checks how it behaves with \#. I need to add that, and replicate the behaviour in strip_comments.

@coveralls
Copy link

coveralls commented Sep 28, 2019

Coverage Status

Coverage decreased (-0.1%) to 40.114% when pulling 6b080e6 on bugfix/652-quoted-pound-sign-treated-as-comment into c63da29 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 40.178% when pulling 6252386 on bugfix/652-quoted-pound-sign-treated-as-comment into c63da29 on master.

@Minoru
Copy link
Member Author

Minoru commented Sep 29, 2019

Turns out tokenize_quoted handled \# already; it was ignored, i.e. it didn't start a comment—just as @noctux expected. After sleeping on this, I too agree with @noctux. I implemented the same behaviour in strip_comments now. Also added a sentence about this to the doc.

@der-lyse's idea proved extremely useful. I realized that double quotes inside backticks shouldn't start a quoted string, and had to fix that.

Backticks take precedence over quotes. For example, option "this `weird " command` for value" is actually valid, and it means:

  1. execute weird " command in a shell. Suppose it prints out a string "OUTPUT";
  2. replace backticks with "OUTPUT", getting option "this OUTPUT for value";
  3. parse that, treating text inside double quotes as a single value. In this case, an option option will be assigned a value of this OUTPUT for value.

Which is counter-intuitive, since double quotes aren't balanced in the original string; yet that's how it works.

Sorry for the messy history, but it took some experimentation to figure out the behaviour of tokenize_quoted and evaluate_backticks, and then some fixes when I realized my understanding is incomplete.

Looking forward for your further reviews!

@Minoru
Copy link
Member Author

Minoru commented Sep 30, 2019

I realized the function doesn't need to return a fancy Cow type—&str is sufficient.

Copy link
Contributor

@der-lyse der-lyse left a comment

Choose a reason for hiding this comment

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

Puh, the tests were really difficult to read and check, especially as the raw strings are delimited with " and #. ;-) I hope nothing slipped through.

Backticks and # characters can be escaped with a backslash (+`+); in that case, they'll be replaced with a literal backtick/# in the configuration.

Just a small thing regarding the docs: You could also cite the escaped pound sign ((+\#+)) along with the escaped backtick ((+\+)`) to make it consistent with the added pound sign at the end of the sentence.

@Minoru
Copy link
Member Author

Minoru commented Oct 1, 2019

Oops, it actually doesn't even render properly ._. Fighting with it now. I'll apply your suggestion as well; thanks!

@noctux, any comments on what I got here? Do you think this is ready for merge?

@Minoru
Copy link
Member Author

Minoru commented Oct 1, 2019

Phew, figured it out. Apparently +\`+ was a wrong way to escape a backtick; it messed up all code fragments that followed it. Since we only generate HTML docs, I replaced it with an HTML entity. Asciidoc replaced it with literal backtick even in HTML, so perhaps it's even portable.

Copy link
Contributor

@noctux noctux left a comment

Choose a reason for hiding this comment

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

Looks good to me now and the tests look convincing.

@Minoru
Copy link
Member Author

Minoru commented Oct 1, 2019

Cool; thank you both very much!

I'll release 2.17.1 tomorrow.

@Minoru Minoru force-pushed the bugfix/652-quoted-pound-sign-treated-as-comment branch from da8f63b to 6b080e6 Compare October 2, 2019 10:41
@Minoru Minoru merged commit b3d0b96 into master Oct 2, 2019
@Minoru Minoru deleted the bugfix/652-quoted-pound-sign-treated-as-comment branch October 2, 2019 13:30
@Minoru Minoru added this to the 2.18 milestone Dec 22, 2019
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.

4 participants