Allow the use of single quote characters in Javadoc task options header and footer by catesq · Pull Request #1288 · gradle/gradle

catesq · 2017-02-01T18:04:16Z

Fix and test for the following issue.

Any of the checked boxes below indicate that I took action:

[x ] Reviewed the Contribution Guidelines.
Signed the Gradle CLA.
Ensured that basic checks pass: ./gradlew quickCheck

The report from languageJava:integtest shows the new test passing successfully.

I tried to make sure the fix doesn't break an existing workaround by trying many combinations of backslashes and delimiters (single, double, triple quoted & slashy strings), all variations failed to build.

gradle#1090

bmuschko · 2017-03-16T14:22:57Z

@mitnuh Thanks for providing the pull request.

I tried to make sure the fix doesn't break an existing workaround by trying many combinations of backslashes and delimiters (single, double, triple quoted & slashy strings), all variations failed to build.

We'd have to make sure existing escape logic still works. Before that we won't merge.

bmuschko · 2017-03-16T14:21:13Z

        file("build/docs/javadoc/Foo.html").text.contains("""Hey Joe!""")
    }

+    @Issue("https://github.com/gradle/gradle/issues/1090")


We are not using the full URL but just gradle/gradle#1090 instead e.g. @Issue("gradle/gradle#1090").

bmuschko · 2017-03-16T14:23:32Z

+    def "escape single quotes in javadoc options"() {
+        buildFile << """
+            apply plugin: "java"
+            javadoc.options.header = "where you goin' with that gun in your hand"


Should also test that multiple occurrences of ' are escaped properly.

bmuschko · 2017-03-16T14:23:57Z

+        file("src/main/java/Foo.java") << "public class Foo {}"
+
+        when: run("javadoc", "-i")
+        then:


There should be a new line between when and then.

Thank you, will fix all the issues you noted.

Regarding succeeds rather than run and a new line between when and then: Several other tests in that file are inconsistent and I will change them if desired. Should I add a separate commit in this PR, a new PR, not change them?

I can touch that up after the the merge. For now just use suceeds for the code you introduced.

Ok, made separate tests for single/double/etc quoted strings and the \ escape sequences which result in a single quote.

Not sure if it's overkill to have multiple tests for this. Can put them in a single test using options.header, options.footer if preferred.

Let's have a single test (the logic will be the same) with a where clause that tests different inputs. When you change the test please also add the @Unroll annotation and indicate some sort of description in the test method name. You will find other examples in the Gradle core code base.

Neat, nice tip, ty.

bmuschko · 2017-03-16T14:24:27Z

+
+        file("src/main/java/Foo.java") << "public class Foo {}"
+
+        when: run("javadoc", "-i")


Should use succeeds instead of run. There's not need for executing with info log level.

eriwen · 2017-03-20T15:26:32Z

@bmuschko I expect you may not have gotten a notification here, but I think this is ready for another review.

bmuschko · 2017-03-21T18:35:01Z

+        file("build/docs/javadoc/Foo.html").text.contains(optionOut)
+
+        where:
+        quote    | optionIn                                   | optionOut


I am a bit confused by all the escaping and some of the naming here. I suggest the following:

We will not use triple, double quotes for the Strings as we'd only need it for multi-line String. That's not the case here. Let's use \" instead. The first test would look as such: "'" | "\"'single quoted'\"" | "'single quoted'"

What exactly do you mean by double or triple quoted? Do you mean the use of 2 vs. 3 quotes within a String? If that's not the case, can you please clean up the test case so it becomes apparent?

When exactly would you use /' to indicate a single quote? Can you clarify?

I was mistakenly trying to test each delimiter rather than testing expected/correct usage, it is confusing and the naming made it worse.

I changed so the test to try and make it clearer and highlight what I was trying to test but the changes meant I couldn't implement your suggestions as requested.

Double or triple quoted meant double quoted or triple - double quoted, was referring to the internal delimiter immediately around the single quotes. The text inside the quotes was only meant as marker test string and making it mean anything was misleading.

Not trying to indicate single quote, have been testing different delimiters because they had slightly different escape patterns I thought I best test them each individually. It complicates the test and makes it unclear what is being tested, starting to wonder whether a single test of options.header="'str'" is sufficient.

I'm tired and misunderstood the triple double quoted comment and removed the third row of the test. Can replace it and submit for another review.

bmuschko · 2017-03-22T13:35:46Z

+        file("build/docs/javadoc/Foo.html").text.contains('"\'header\'"')
+
+        where:
+        delimiter | header


Sorry, I still don't fully understand the test case we are running though here. Can you please explain? Maybe should just have the simplest test case that was actually reported: the use of single quote characters. The other two test case I do not get. For example when would you want to use "/"? The other test case is just verifying that Groovy can use single or double quotes to define a String. Maybe I just misunderstood your initial comment about escaping Strings.

You're right. I misunderstood what made a good test and you've been trying to understand my mistake. The simplest test case would be best, mine were the same testing the exact same thing in four different ways for no reason.

No problem. I am going to touch up the test after merging.

bmuschko · 2017-03-23T13:00:56Z

Thanks for the pull request. It has been merged!

…eproducibility Let generated extensions jar be reproducible

Allow single quote characters in javadoc task options.

3fea781

gradle#1090

eriwen added cla:yes labels Feb 2, 2017

bmuschko suggested changes Mar 16, 2017

View reviewed changes

bmuschko self-assigned this Mar 16, 2017

bmuschko added waiting-for:contributor and removed waiting-for:gradle-team labels Mar 16, 2017

catesq added 2 commits March 17, 2017 16:26

fix: changes requested on pr gradle#1288

18976e7

Use @unroll

dd1df67

eriwen added waiting-for:gradle-team and removed waiting-for:contributor labels Mar 20, 2017

bmuschko suggested changes Mar 21, 2017

View reviewed changes

bmuschko added waiting-for:contributor and removed waiting-for:gradle-team labels Mar 21, 2017

changes test for pr gradle#1288

b0c552f

bmuschko suggested changes Mar 22, 2017

View reviewed changes

single quote test

0a6e7fa

bmuschko merged commit 9e5363f into gradle:master Mar 23, 2017

bmuschko changed the title ~~Allow single quote characters in javadoc task options.~~ Allow the use of single quote characters in Javadoc task options header and footer Mar 23, 2017

bmuschko added this to the 4.0 RC1 milestone Mar 23, 2017

bmuschko removed the waiting-for:contributor label Mar 23, 2017

bot-plugin-portal pushed a commit that referenced this pull request Feb 7, 2019

Merge pull request #1288 from gradle/eskatos/caching/generated-code-r…

320ec73

…eproducibility Let generated extensions jar be reproducible


		file("src/main/java/Foo.java") << "public class Foo {}"

		when: run("javadoc", "-i")

Uh oh!

Conversation

catesq commented Feb 1, 2017

Uh oh!

bmuschko commented Mar 16, 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!

eriwen commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmuschko Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bmuschko commented Mar 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmuschko Mar 22, 2017 •

edited

Loading