Added Capitalized #1465

kokodyn · 2020-09-18T13:28:50Z

Added EO version of StringUtils.capitalize

I hope for your understanding, this is my first pull request to open source project.
Signed-off-by: kokodyn [email protected]

Added EO version of StringUtils.capitalize Signed-off-by: kokodyn <[email protected]>

andreoss

@kokodyn Please see my comments.
@victornoel There's no issue attached to this PR, should we create one?

andreoss · 2020-09-18T13:49:28Z

src/main/java/org/cactoos/text/Capitalized.java

+        super(
+            new TextOf(
+                () -> {
+                    String capitalized = text.asString();


@kokodyn Let's remove mutable variable from here. I guess Ternary can be used

Done but I am not fully convinced to using Ternary. It seems be more complicated than previous version.

andreoss · 2020-09-18T13:49:52Z

src/test/java/org/cactoos/text/CapitalizedTest.java

+    @Test
+    public void capitalizeEmptyText() {
+        new Assertion<>(
+            "Can't capitalize an empty text",


@kokodyn I suggest must instead of cannot

I prefer to use "Can't" because as I see it it used commonly in other tests in similar context.

@kokodyn please let's use must as it is the default we use in cactoos (there are still examples where can't is used but this should be changed bit by bit)

andreoss · 2020-09-18T13:51:16Z

src/main/java/org/cactoos/text/Capitalized.java

+ *
+ * @since 0.46
+ */
+public class Capitalized extends TextEnvelope {


@kokodyn May be we should name it Title instead?
Should it make all non-first characters lower-cased?

I will create ticket for "Capitalized" issue and we can discuss further,

I created issue for this:
#1466

andreoss · 2020-09-18T13:51:45Z

src/main/java/org/cactoos/text/Capitalized.java

+ *
+ * @since 0.46
+ */
+public class Capitalized extends TextEnvelope {


@kokodyn The class should be final

andreoss · 2020-09-18T13:52:12Z

src/test/java/org/cactoos/text/CapitalizedTest.java

+ */
+package org.cactoos.text;
+
+import org.junit.Test;


@kokodyn Please use Junit 5 and make class final, but not public

I hope I did it in proper way, I need to update path to @test annotation to allow Eclipse to handle it.

andreoss · 2020-09-18T13:53:44Z

src/test/java/org/cactoos/text/CapitalizedTest.java

+    /**
+     * Text starting with upper case character.
+     */
+    private static final String UPPER_CASE = "Abc";


@kokodyn I believe this constant is not necessary. If it causes Qulice complain, it would be better to suppress that.

Signed-off-by: kokodyn <[email protected]>

codecov-commenter · 2020-09-18T17:43:01Z

Codecov Report

Merging #1465 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1465      +/-   ##
============================================
+ Coverage     89.28%   89.34%   +0.06%     
+ Complexity     1661     1629      -32     
============================================
  Files           278      277       -1     
  Lines          3965     3904      -61     
  Branches        209      209              
============================================
- Hits           3540     3488      -52     
+ Misses          392      383       -9     
  Partials         33       33

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/text/Capitalized.java	`100.00% <100.00%> (ø)`	`5.00 <5.00> (?)`
src/main/java/org/cactoos/scalar/Solid.java	`90.00% <0.00%> (-10.00%)`	`3.00% <0.00%> (-1.00%)`
src/main/java/org/cactoos/map/Synced.java	`28.57% <0.00%> (-8.93%)`	`3.00% <0.00%> (-1.00%)`
src/main/java/org/cactoos/map/MapEnvelope.java	`91.66% <0.00%> (-6.12%)`	`15.00% <0.00%> (-15.00%)`
src/main/java/org/cactoos/map/MapOf.java	`100.00% <0.00%> (ø)`	`10.00% <0.00%> (-1.00%)`
src/main/java/org/cactoos/map/Merged.java	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (ø%)`
src/main/java/org/cactoos/map/Grouped.java	`100.00% <0.00%> (ø)`	`1.00% <0.00%> (-1.00%)`
src/main/java/org/cactoos/map/Solid.java
src/main/java/org/cactoos/map/Sticky.java

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af979de...09b9113. Read the comment docs.

andreoss

@kokodyn There's more

andreoss · 2020-09-18T19:48:03Z

src/test/java/org/cactoos/text/CapitalizedTest.java

@@ -68,8 +65,8 @@ public void capitalizeSingleUpperCaseText() {
    public void capitalizeTextStartingWithUpperCaseCharacter() {


@kokodyn public is not necessary

andreoss · 2020-09-18T20:43:16Z

src/main/java/org/cactoos/text/Capitalized.java

+        super(
+            new TextOf(
+                () -> {
+                    final Func<String, String> capitalize = cap -> {


@kokodyn
I believe it should be more like:

super(new Ternary<>(new IsBlank(text), () -> text, new Ternary<>(new StartsWith(...), ...);

Also, Func<String, String> should be a Text but without extra checks

Ok. Not sure what does it mean "without extra checks". Please look at my proposition in newest commit.

@kokodyn It looks complicated.
Why it cannot be like that?

new Ternary<Text>( new IsBlank(text), text, new Joined( "", new Upper(new Sub(text, 0, 1)), new Sub(text, 1) ) )

May be imperative approach was better here.

@andreoss While working on that PR I realized that there is a difference between Upper and TitleCase:
https://stackoverflow.com/questions/31770995/difference-between-uppercase-and-titlecase

@kokodyn I agree with @andreoss, why not use the much simpler example he proposes?

no need for testing if the first character is already title case or not, let's just do it as in @andreoss example above

let's use Upper for now

let's add a @todo to the class so that we add Titled and replace Upper in this class.

@kokodyn that's acceptable, but please take note that the purpose of cactoos is to be a sandbox area for experimenting with Zerocracy, on top of implementing this library. Hence in the future it would be best to follow the proper workflow which advocates for using @todos as a way to decompose work. Once from time to time, it is ok to not follow this, but this should remain the general nominal way of working here.

@victornoel
i added @todo comment to with information what should be replaced in future.
I would like to have it proper working(and later replaced using by "EO Titled" ) instead of using Lower.
As I said before this is tricky difference beetwen Lower and Titled.
For instance:
Title case: ǅ ->https://www.compart.com/en/unicode/U+01C5
Lower case: ǆ -> https://www.compart.com/en/unicode/U+01C6
Upper case: Ǆ -> https://www.compart.com/en/unicode/U+01C4

However, if you are sure that Lower should be used please let me know.

@kokodyn I thought we were talking about Upper vs Titled, no? What I propose is to tackle this issue incrementally, so first we make it work with Upper because it's good enough, and then with the todo we explore how to have Capitalized use titled characters.

@kokodyn anywa, see my last comment about the current code, maybe we can just use title case as-is right now without introducing any new class.

@victornoel Yes we can use tittle case without introducing any more class.

Signed-off-by: kokodyn <[email protected]>

victornoel

@kokodyn here are some comments :) all of this seems a bit complex for now, let's see if we can simplify it bit by bit!

victornoel · 2020-09-22T07:58:24Z

src/main/java/org/cactoos/text/Capitalized.java

+                            );
+                        return
+                            new Ternary<Text>(
+                                () -> new StartsWith(cap, titled).value(),


@kokodyn I believe you can use directly StartsWith instead of passing it as a lambda since it implements Scalar<Boolean>.

@victornoel done

victornoel · 2020-09-22T07:58:59Z

src/main/java/org/cactoos/text/Capitalized.java

+                        final Text titled = new TextOf(
+                            Character.toChars(
+                                Character.toTitleCase(
+                                    cap.asString().codePointAt(0)
+                                    )
+                                )
+                            );


@kokodyn there is something strange with indentation here

@victornoel I hope now it looks better.

src/main/java/org/cactoos/text/Capitalized.java

victornoel · 2020-09-22T08:01:32Z

src/main/java/org/cactoos/text/Capitalized.java

+                    };
+                    return new Ternary<Text>(
+                        text,
+                        t -> new IsBlank(t).value(),


@kokodyn here too you can directly pass IsBlank instead of a lambda

@victornoel done

Signed-off-by: kokodyn <[email protected]>

victornoel · 2020-09-27T15:14:27Z

src/main/java/org/cactoos/text/Capitalized.java

+                            new TextOf(
+                                Character.toChars(
+                                    Character.toTitleCase(
+                                        new Sub(text, 0, 1).asString().codePointAt(0)


@kokodyn I believe you can directly write new TextOf(Character.toTitleCase(text.asString().codePointAt(0))) no?

@victornoel
Good point!
new TextOf(Character.toChars(Character.toTitleCase(text.asString().codePointAt(0))))
I believe this is the easiest version

Signed-off-by: kokodyn <[email protected]>

victornoel · 2020-09-27T15:50:40Z

@kokodyn I feel like we arrived to a good result :)

@andreoss could you take a last look to see if I haven't overlooked anything? thx

victornoel · 2020-10-02T17:50:32Z

@andreoss could you finish the review?

andreoss · 2020-10-03T00:31:57Z

src/main/java/org/cactoos/text/Capitalized.java

+                                )
+                            ),
+                            new Sub(text, 1)
+                            )


@kokodyn The indentation looks odd here. And at 70 as well.
You can also use lambda without return here.

andreoss · 2020-10-03T00:32:46Z

@victornoel Everything but the indentation looks good.

victornoel · 2020-10-04T11:11:43Z

@andreoss thx, I will merge as-is since it is not critical and I will most certainly touch this same code when solving #1460 :)

victornoel · 2020-10-04T11:11:51Z

@rultor merge

victornoel · 2020-10-04T11:12:02Z

@kokodyn thx for your patience

rultor · 2020-10-04T11:12:54Z

@rultor merge

@victornoel OK, I'll try to merge now. You can check the progress of the merge here

rultor · 2020-10-04T11:22:04Z

@rultor merge

@victornoel Done! FYI, the full log is here (took me 9min)

0crat · 2020-10-04T12:01:25Z

Code review was too long (15 days), architects (@victornoel) were penalized, see §55

0crat · 2020-10-04T12:01:42Z

@sereshqua /z please review this job completed by @andreoss /z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

sereshqua · 2020-10-04T12:34:33Z

@kokodyn please make sure you start all your comments with the name of the user they are referred to, see

sereshqua · 2020-10-04T12:34:43Z

@0crat quality acceptable

kokodyn · 2020-10-06T20:30:45Z

@victornoel @andreoss @sereshqua
Thanks for participate, comments etc.
i will try to make better PR more and more.
BR,
kokodyn

Added Capitalized

e0d374b

Added EO version of StringUtils.capitalize Signed-off-by: kokodyn <[email protected]>

0crat added 0crat/scope 0crat/role/REV labels Sep 18, 2020

andreoss suggested changes Sep 18, 2020

View reviewed changes

add using Ternary and other fixes

d9a55ab

Signed-off-by: kokodyn <[email protected]>

andreoss suggested changes Sep 18, 2020

View reviewed changes

#1465: changed Upper to Character.toTitleCase and other fixes

06a1c65

Signed-off-by: kokodyn <[email protected]>

victornoel requested changes Sep 22, 2020

View reviewed changes

victornoel mentioned this pull request Sep 22, 2020

Add Capitalized EE version of StringUtils.capitalize #1466

Closed

kokodyn added 2 commits September 22, 2020 22:54

#1465: add using Joined and other simplifications

114a646

Signed-off-by: kokodyn <[email protected]>

simplify logic and use upper

03c34da

Signed-off-by: kokodyn <[email protected]>

victornoel mentioned this pull request Sep 27, 2020

#1466 Added titled #1474

Closed

add TODO comment to replace using Character#toTitleCase(int) in future

33c1c2c

Signed-off-by: kokodyn <[email protected]>

victornoel reviewed Sep 27, 2020

View reviewed changes

simplify logic

09b9113

Signed-off-by: kokodyn <[email protected]>

andreoss suggested changes Oct 3, 2020

View reviewed changes

rultor merged commit 36dd67b into yegor256:master Oct 4, 2020

0crat added the qa label Oct 4, 2020

0crat removed the 0crat/scope label Oct 4, 2020

0crat added quality/acceptable and removed qa labels Oct 4, 2020

kokodyn deleted the CAPITALIZED branch October 7, 2020 21:35

		@@ -68,8 +65,8 @@ public void capitalizeSingleUpperCaseText() {
		public void capitalizeTextStartingWithUpperCaseCharacter() {

Added Capitalized #1465

Added Capitalized #1465

Conversation

kokodyn commented Sep 18, 2020

andreoss left a 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

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

codecov-commenter commented Sep 18, 2020 • edited Loading

Codecov Report

andreoss left a 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

kokodyn Sep 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokodyn Sep 27, 2020 • edited Loading

Choose a reason for hiding this comment

victornoel left a 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

kokodyn Sep 27, 2020 • edited Loading

Choose a reason for hiding this comment

victornoel commented Sep 27, 2020

victornoel commented Oct 2, 2020

Choose a reason for hiding this comment

andreoss commented Oct 3, 2020

victornoel commented Oct 4, 2020

victornoel commented Oct 4, 2020

victornoel commented Oct 4, 2020

rultor commented Oct 4, 2020

rultor commented Oct 4, 2020

0crat commented Oct 4, 2020

0crat commented Oct 4, 2020

sereshqua commented Oct 4, 2020

sereshqua commented Oct 4, 2020

kokodyn commented Oct 6, 2020

codecov-commenter commented Sep 18, 2020 •

edited

Loading

kokodyn Sep 27, 2020 •

edited

Loading

kokodyn Sep 27, 2020 •

edited

Loading

kokodyn Sep 27, 2020 •

edited

Loading