Implementation of "Filler Words" #472

richard-stowe · 2020-06-17T13:12:46Z

I have the first PR ready. In addition to the new @FillerWord concept I've also added @Punctuation, let me know what you think?

Richard

jgiven-core/src/main/java/com/tngtech/jgiven/annotation/FillerWord.java

jgiven-core/src/main/java/com/tngtech/jgiven/annotation/Punctuation.java

l-1squared · 2020-06-17T14:28:34Z

jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java

 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.Stack;

+import static com.google.common.collect.Iterables.concat;
+import static com.google.common.collect.Iterables.getLast;


if we start introducing google.common.collect as static import, all used methods should be imported statically and the imports in line 5 and 6 should be removed.

Muscle memory pulled in the static imports. I've reverted this and aligned new usages of google.common.collect with the previous implementation

l-1squared · 2020-06-17T14:44:10Z

jgiven-core/src/main/java/com/tngtech/jgiven/report/model/Word.java

+        append( word, " " );
+    }
+
+    public void append( CharSequence word, CharSequence delimiter ) {


I'm not entirely certain, if having a generic method is the best approach here. From my understanding we have to classes of things to add here: Words and Punctuation. As I see it words should always be appended with space, punctuation always without. Admittedly there are special characters like [(" where that might not be true, but I don't think they are really supported.
Hence my suggestion is to have two method appendWord and appendPunctuation that always add the input with or without whitespace respectively.
In my opinon this would make is easier for readers to understand the usage of the methods.

I've incorporated your suggestion

l-1squared · 2020-06-17T14:47:27Z

jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java

@@ -159,6 +171,18 @@ public void introWordAdded( String value ) {
        introWord.setValue( value );
    }

+    private void fillerWordAdded( String value ) {


I prefer addToFillerWords, because with this name, I always expect this to be a test that returns a boolean.

jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java

l-1squared · 2020-06-17T15:06:26Z

jgiven-core/src/main/java/com/tngtech/jgiven/impl/util/PrintWriterUtil.java

+            return new PrintWriter( file, Charsets.UTF_8.name() ) {
+                @Override
+                public void println() {
+                    write( "\n" );


why is that better than the code of the standard PrintWriter class? Also, if there is a reason to circumvent the double checked locking in there, wouldn't you at least want to use this.lineSeparator instead of a hardcoded \n

Ah, this is something I quickly did to get the build working on my windows machine. I'll revert and look for something a little less "hacky"

jgiven-core/src/test/java/com/tngtech/jgiven/impl/ScenarioModelBuilderTest.java

l-1squared · 2020-06-17T15:16:16Z

...s/src/test/java/com/tngtech/jgiven/examples/pancakes/test/ExtendedVocabularyPanCakeTest.java

+    public void a_pancake_can_be_fried_out_of_an_egg_milk_and_flour() {
+
+        given() .ingredients().comma() .consisting_of()
+            .asterisk() .an() .egg()


in this case, I'd argue that asterisk is functionally closer to a word than to punctuation. But I get now, why you implemented it the way you did. I'm just not completely satisfied...

Ok, agreed. Maybe I overdid this one. I'll wind back the use of punctuation a little.

docs/stages.adoc

jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java

l-1squared · 2020-06-18T13:40:46Z

keeping this open for another day or say to give @janschaefer another opportunity to voice his opinion

janschaefer · 2020-06-19T04:47:06Z

I think this is a great addition to JGiven, so thanks for that! I have two things:

Have you verified that this works as expected in the HTML report as well?
I personally had named the attribute punctuation of the FillerWord more technical. Something like: noWhitespace or just whitespace which is true by default. Because there might be situations in which you maybe have a filler word that is no punctuation and still want to have no white space.

richard-stowe · 2020-06-19T06:46:15Z

@janschaefer, good point. I'm not completely sold on whitespace though, how about affix?

richard-stowe · 2020-06-19T10:06:00Z

HTML reports look good:

richard-stowe · 2020-06-19T11:05:21Z

@janschaefer, good point. I'm not completely sold on whitespace though, how about affix?

I've implemented this now. Let me know what you think.

jgiven-core/src/main/java/com/tngtech/jgiven/annotation/FillerWord.java

codecholeric · 2020-06-20T12:42:58Z

BTW: Those "addressing feedback", "rename x" of "fixed typo" commits I would squash onto the commits where the original change was introduced. Don't think they bring much value in the master history, right?

jgiven-core/src/main/java/com/tngtech/jgiven/impl/SentenceBuilder.java

docs/stages.adoc

jgiven-core/src/test/java/com/tngtech/jgiven/GivenTestStep.java

Signed-off-by: Richard Stowe <richard.stowe@bt.com>

outdated

jgiven-core/src/main/java/com/tngtech/jgiven/impl/SentenceBuilder.java

richard-stowe force-pushed the filler-words branch from 2b376f5 to d585219 Compare June 17, 2020 13:13

l-1squared reviewed Jun 17, 2020

View reviewed changes

richard-stowe force-pushed the filler-words branch 2 times, most recently from ffed081 to d0f5029 Compare June 18, 2020 07:57

l-1squared previously approved these changes Jun 18, 2020

View reviewed changes

docs/stages.adoc Show resolved Hide resolved

jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java Outdated Show resolved Hide resolved

richard-stowe force-pushed the filler-words branch from 0275d20 to 8872852 Compare June 18, 2020 14:35

richard-stowe changed the title ~~Implementation of "Filler Words" and "Punctuation"~~ Implementation of "Filler Words" Jun 19, 2020

richard-stowe force-pushed the filler-words branch 3 times, most recently from 124efeb to d475362 Compare June 19, 2020 10:59

l-1squared reviewed Jun 19, 2020

View reviewed changes

jgiven-core/src/main/java/com/tngtech/jgiven/annotation/FillerWord.java Outdated Show resolved Hide resolved

richard-stowe force-pushed the filler-words branch from 8d9c7d0 to c0651ef Compare June 22, 2020 15:56

l-1squared reviewed Jun 23, 2020

View reviewed changes

l-1squared self-requested a review June 23, 2020 05:05

Implementation of "Filler Words"

4c01ccc

Signed-off-by: Richard Stowe <richard.stowe@bt.com>

richard-stowe force-pushed the filler-words branch from c0651ef to 4c01ccc Compare June 23, 2020 06:36

l-1squared approved these changes Jun 23, 2020

View reviewed changes

jgiven-core/src/main/java/com/tngtech/jgiven/impl/SentenceBuilder.java Show resolved Hide resolved

l-1squared merged commit 6df8c0e into TNG:master Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of "Filler Words" #472

Implementation of "Filler Words" #472

richard-stowe commented Jun 17, 2020

l-1squared Jun 17, 2020

richard-stowe Jun 17, 2020

l-1squared Jun 17, 2020

richard-stowe Jun 17, 2020

l-1squared Jun 17, 2020

richard-stowe Jun 17, 2020

l-1squared Jun 17, 2020

richard-stowe Jun 17, 2020

l-1squared Jun 17, 2020

richard-stowe Jun 17, 2020

l-1squared commented Jun 18, 2020

janschaefer commented Jun 19, 2020

richard-stowe commented Jun 19, 2020

richard-stowe commented Jun 19, 2020

richard-stowe commented Jun 19, 2020

codecholeric commented Jun 20, 2020 •

edited

Loading

Implementation of "Filler Words" #472

Implementation of "Filler Words" #472

Conversation

richard-stowe commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-1squared commented Jun 18, 2020

janschaefer commented Jun 19, 2020

richard-stowe commented Jun 19, 2020

richard-stowe commented Jun 19, 2020

richard-stowe commented Jun 19, 2020

codecholeric commented Jun 20, 2020 • edited Loading

codecholeric commented Jun 20, 2020 •

edited

Loading