-
Notifications
You must be signed in to change notification settings - Fork 126
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
API to disable span support #24
base: master
Are you sure you want to change the base?
Conversation
796b309
to
3f8ace2
Compare
@@ -67,6 +68,12 @@ | |||
private char curChar; | |||
private int curCharIndex; | |||
|
|||
/** | |||
* When enabled, span support is disabled. This can be useful for unit tests that don't want |
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.
might need a bit help to word this better.
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.
first sentence is redundant with field name. Just keep the second sentence ("This can be useful...").
3f8ace2
to
42ef4ee
Compare
@@ -214,6 +234,12 @@ public CharSequence format() { | |||
return formatted; | |||
} | |||
|
|||
/** Set whether span support is enabled or not. */ | |||
@SuppressWarnings("UnusedDeclaration") // Public API. | |||
public void setDisableSpanSupport(boolean disableSpanSupport) { |
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.
invert: setSpanSupportEnabled
. default to true.
javadoc should indicate default. move javadoc from field to this method.
42ef4ee
to
03832d3
Compare
import static java.util.regex.Matcher.quoteReplacement; | ||
import static java.util.regex.Pattern.quote; | ||
|
||
public class SimpleEditable implements Editable { |
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.
final. definitely not public. needs javadoc.
03832d3
to
40375f6
Compare
* Default is <code>true</code>. | ||
*/ | ||
@SuppressWarnings("UnusedDeclaration") // Public API. | ||
public void setSpanSupportEnabled(boolean spanSupportedEnabled) { |
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.
Have you considered just making this variable final, and having new factory methods for it? There'd be a lot of method proliferation I guess.
I was thinking of something like Phrase.stubSpan().from(...)
or Phrase.stubSpanFrom(...)
or something. That way you just have a small amount of code to add across a lot of tests.
I'd just like someone to avoid running into this method in their normal Phrase usage.
LGTM |
/** | ||
* This can be useful for unit tests that don't want to use {@link SpannableStringBuilder} | ||
* which can cause a stub exception or be no-op. | ||
* Default is <code>true</code>. |
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 should change that since I switched it, it should say <code>false</code>
bdc9261
to
d57d3d8
Compare
We should merge this, just noticed we've been using it for a looong time right off this commit. We may want to pursue some sort of "default" span enabled setting that would enable even easier test writing |
d57d3d8
to
ad818ca
Compare
a876d9b
to
663ca14
Compare
@edenman we should get this in for a release |
@dnkoutso want to rebase? |
This mode unlocks the ability for consumers to disable span support. This enables writing pure unit tests that do not depend on
SpannableStringBuilder
which otherwise would require Robolectric to run and make the tests much much slower.Fixed all warnings too.
@loganj @ChrisRenke @rjrjr