-
Notifications
You must be signed in to change notification settings - Fork 98
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
Implementation of "Filler Words" #472
Conversation
2b376f5
to
d585219
Compare
jgiven-core/src/main/java/com/tngtech/jgiven/annotation/FillerWord.java
Outdated
Show resolved
Hide resolved
jgiven-core/src/main/java/com/tngtech/jgiven/annotation/Punctuation.java
Outdated
Show resolved
Hide resolved
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muscle memory pulled in the static imports. I've reverted this and aligned new usages of google.common.collect
with the previous implementation
append( word, " " ); | ||
} | ||
|
||
public void append( CharSequence word, CharSequence delimiter ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've incorporated your suggestion
@@ -159,6 +171,18 @@ public void introWordAdded( String value ) { | |||
introWord.setValue( value ); | |||
} | |||
|
|||
private void fillerWordAdded( String value ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer addToFillerWords
, because with this name, I always expect this to be a test that returns a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java
Outdated
Show resolved
Hide resolved
return new PrintWriter( file, Charsets.UTF_8.name() ) { | ||
@Override | ||
public void println() { | ||
write( "\n" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
public void a_pancake_can_be_fried_out_of_an_egg_milk_and_flour() { | ||
|
||
given() .ingredients().comma() .consisting_of() | ||
.asterisk() .an() .egg() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, agreed. Maybe I overdid this one. I'll wind back the use of punctuation a little.
ffed081
to
d0f5029
Compare
jgiven-core/src/main/java/com/tngtech/jgiven/impl/ScenarioModelBuilder.java
Outdated
Show resolved
Hide resolved
keeping this open for another day or say to give @janschaefer another opportunity to voice his opinion |
0275d20
to
8872852
Compare
I think this is a great addition to JGiven, so thanks for that! I have two things:
|
@janschaefer, good point. I'm not completely sold on |
124efeb
to
d475362
Compare
I've implemented this now. Let me know what you think. |
jgiven-core/src/main/java/com/tngtech/jgiven/annotation/FillerWord.java
Outdated
Show resolved
Hide resolved
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 |
8d9c7d0
to
c0651ef
Compare
jgiven-core/src/main/java/com/tngtech/jgiven/impl/SentenceBuilder.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Richard Stowe <richard.stowe@bt.com>
c0651ef
to
4c01ccc
Compare
Hi @janschaefer and @l-1squared.
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