Skip to content
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

(#1574) Generify org.cactoos.text #1620

Merged

Conversation

DmitryBarskov
Copy link
Contributor

For #1574:

  • Use CharSequence instead of String when possible
  • Generic covariance for Flattened
  • Wildcards for Collection if type parameter doesn't matter
  • Counter-variance for Func

src/main/java/org/cactoos/text/Replaced.java Outdated Show resolved Hide resolved
src/main/java/org/cactoos/text/Strict.java Show resolved Hide resolved
src/main/java/org/cactoos/text/Sub.java Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1620 (85a26bd) into master (cc5daf0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1620      +/-   ##
============================================
+ Coverage     90.07%   90.10%   +0.02%     
- Complexity     1602     1603       +1     
============================================
  Files           298      298              
  Lines          3748     3748              
  Branches        122      122              
============================================
+ Hits           3376     3377       +1     
  Misses          338      338              
+ Partials         34       33       -1     
Impacted Files Coverage Δ
src/main/java/org/cactoos/text/Abbreviated.java 100.00% <ø> (ø)
src/main/java/org/cactoos/text/Flattened.java 100.00% <ø> (ø)
src/main/java/org/cactoos/text/FormattedText.java 100.00% <ø> (ø)
src/main/java/org/cactoos/text/Joined.java 100.00% <ø> (ø)
src/main/java/org/cactoos/text/Mapped.java 100.00% <ø> (ø)
src/main/java/org/cactoos/text/Strict.java 100.00% <ø> (ø)
src/main/java/org/cactoos/text/Sub.java 86.20% <ø> (ø)
src/main/java/org/cactoos/text/Replaced.java 100.00% <100.00%> (ø)
src/main/java/org/cactoos/scalar/Solid.java 100.00% <0.00%> (+10.00%) ⬆️

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 cc5daf0...85a26bd. Read the comment docs.

Copy link
Collaborator

@victornoel victornoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DmitryBarskov thx for the tests, I had the wrong intuition about this ? super String!

src/main/java/org/cactoos/text/FormattedText.java Outdated Show resolved Hide resolved
src/test/java/org/cactoos/text/SubTest.java Outdated Show resolved Hide resolved
src/test/java/org/cactoos/text/StrictTest.java Outdated Show resolved Hide resolved
@victornoel
Copy link
Collaborator

@DmitryBarskov so one last check: did you validate that there were no more generic to update in the whole text package?

@DmitryBarskov
Copy link
Contributor Author

@victornoel found some other parameters for generalizing. But I skipped some params when they are regular expressions. I think they should be string literals. WDYT?

@victornoel
Copy link
Collaborator

@DmitryBarskov sorry, I'm missing a bit of context: which one are you talking about that should be String instead of CharSequence?

@DmitryBarskov
Copy link
Contributor Author

@victornoel I'm talking about ctor Split(final Text text, final String rgx, final int lmt). Do you think CharSequence is used for regular expressions?

@victornoel
Copy link
Collaborator

@DmitryBarskov I think it's not the responsibility of Split to know how its input are built, so yes, if we decide that the interface to use for string (not the java type, the concept) is CharSequence, then it should be used everywhere.

@victornoel
Copy link
Collaborator

@DmitryBarskov sorry for the late feedback, I didn't realize you made some changes :) don't hesitate to ping me if you don't get any updates in the future!

@victornoel
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Sep 5, 2021

@rultor merge

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

@rultor rultor merged commit 445cf16 into yegor256:master Sep 5, 2021
@rultor
Copy link
Collaborator

rultor commented Sep 5, 2021

@rultor merge

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

@victornoel
Copy link
Collaborator

@0crat status

@DmitryBarskov DmitryBarskov deleted the issue-1574-generify-text-package branch September 6, 2021 08:39
@victornoel
Copy link
Collaborator

@0crat status

@0crat
Copy link
Collaborator

0crat commented Sep 12, 2021

@0crat status (here)

@victornoel This is what I know about this job in C63314D6Z, as in §32:

@victornoel
Copy link
Collaborator

@0crat out

@0crat 0crat added the qa label Sep 12, 2021
@0crat
Copy link
Collaborator

0crat commented Sep 12, 2021

@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

@0crat
Copy link
Collaborator

0crat commented Sep 12, 2021

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

@0crat 0crat removed the 0crat/scope label Sep 12, 2021
@sereshqua
Copy link

@0crat quality good

@0crat 0crat added quality/good and removed qa labels Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants