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

(#1116) Extends TextEnvelope when possible #1131

Merged
merged 1 commit into from
May 31, 2019

Conversation

victornoel
Copy link
Collaborator

This is for #1116.

I've applied the new pattern for 3 classes and added a todo to continue the work.

@0crat
Copy link
Collaborator

0crat commented May 25, 2019

Job #1131 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented May 25, 2019

This pull request #1131 is assigned to @vzurauskas/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

@codecov-io
Copy link

codecov-io commented May 25, 2019

Codecov Report

Merging #1131 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1131   +/-   ##
=========================================
  Coverage     88.83%   88.83%           
  Complexity     1581     1581           
=========================================
  Files           272      272           
  Lines          3931     3931           
  Branches        215      215           
=========================================
  Hits           3492     3492           
  Misses          386      386           
  Partials         53       53
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/text/Randomized.java 100% <ø> (ø) 13 <0> (ø) ⬇️
src/main/java/org/cactoos/text/FormattedText.java 100% <100%> (ø) 8 <1> (ø) ⬇️
src/main/java/org/cactoos/text/NoNulls.java 100% <100%> (ø) 1 <1> (ø) ⬇️
src/main/java/org/cactoos/text/Synced.java 100% <100%> (ø) 2 <1> (ø) ⬇️

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 74a9a55...4a301a9. Read the comment docs.

@victornoel
Copy link
Collaborator Author

@vzurauskas ping ? :)

Copy link
Contributor

@vzurauskas vzurauskas left a comment

Choose a reason for hiding this comment

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

@victornoel Review done.

);
}
return out.toString();
super((Scalar<String>) () -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Since type casting is discouraged in EO, consider adding a private constructor which would take Scalar<String> (or just Text) and call super from there.

);
}
return string;
super((Scalar<String>) () -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Same comment about type casting.

@@ -58,15 +48,10 @@ public Synced(final Text text) {
* @param lck The lock
*/
public Synced(final Text text, final Object lck) {
this.origin = text;
this.lock = lck;
super((Scalar<String>) () -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Same comment about type casting.

@victornoel victornoel force-pushed the 1116-textenvelope-subclasses branch from ba66fa7 to 4a301a9 Compare May 30, 2019 10:30
@victornoel
Copy link
Collaborator Author

@vzurauskas thanks you, in the end I went with using anonymous classes (which is exactly equivalent to a lambda apart from being explicit in the type) to solve this issue because for example NoNulls, I couldn't use another constructor since it would be ambiguous with the one taking a Text.

Copy link
Contributor

@vzurauskas vzurauskas left a comment

Choose a reason for hiding this comment

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

@llorllale Looks good.

@llorllale
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 31, 2019

@rultor merge

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

@rultor rultor merged commit 4a301a9 into yegor256:master May 31, 2019
@rultor
Copy link
Collaborator

rultor commented May 31, 2019

@rultor merge

@llorllale Done! FYI, the full log is here (took me 13min)

@0crat
Copy link
Collaborator

0crat commented May 31, 2019

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

@0crat 0crat removed the scope label May 31, 2019
@0crat
Copy link
Collaborator

0crat commented May 31, 2019

Job #1131 is not in the agenda of @vzurauskas/z, can't inspect

@0crat
Copy link
Collaborator

0crat commented May 31, 2019

The job #1131 is now out of scope

@0crat
Copy link
Collaborator

0crat commented May 31, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale/z

@sereshqua
Copy link

@0crat quality good

@0crat
Copy link
Collaborator

0crat commented Jun 1, 2019

Order was finished, quality is "good": +20 point(s) just awarded to @vzurauskas/z

@0crat
Copy link
Collaborator

0crat commented Jun 1, 2019

Quality review completed: +4 point(s) just awarded to @sereshqua/z

@scristalli
Copy link
Contributor

@llorllale is it possible to make a release from master, or at least one including these changes? The new structure of FormattedText is much better, and I would prefer to use it in cases like this issue, instead of using the old version and refactor later.
Thanks.

@llorllale
Copy link
Contributor

@scristalli I don't see the difference?

@scristalli
Copy link
Contributor

@llorllale if I'm not mistaken, in 0.41 FormattedText only has a asString() throws Exception method, while with new modifications it will expose both asString() throws IOException, and un unchecked toString() wrapping asString().
In the project I linked we have some methods that already throw IOException and I think it's less confusing if we don't enlarge the range of exceptions just for a string formatting operation.

@llorllale
Copy link
Contributor

@rultor release, tag is 0.42

@rultor
Copy link
Collaborator

rultor commented Jul 4, 2019

@rultor release, tag is 0.42

@llorllale OK, I will release it now. Please check the progress here

@rultor
Copy link
Collaborator

rultor commented Jul 4, 2019

@rultor release, tag is 0.42

@llorllale @victornoel Oops, I failed. You can see the full log here (spent 8min)

	at org.apache.maven.cli.MavenCli.execute(MavenCli.java:863)
	at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:288)
	at org.apache.maven.cli.MavenCli.main(MavenCli.java:199)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:289)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:229)
	at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:415)
	at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:356)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 06:01 min
[INFO] Finished at: 2019-07-04T01:47:48+00:00
[INFO] Final Memory: 50M/530M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.6.0:exec (default) on project cactoos: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.6.0:exec (default) on project cactoos: Command execution failed.
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:212)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:116)
	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:80)
	at org.apache.maven.lifecycle.internal.builder.singlethreaded.SingleThreadedBuilder.build(SingleThreadedBuilder.java:51)
	at org.apache.maven.lifecycle.internal.LifecycleStarter.execute(LifecycleStarter.java:128)
	at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:307)
	at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:193)
	at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:106)
	at org.apache.maven.cli.MavenCli.execute(MavenCli.java:863)
	at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:288)
	at org.apache.maven.cli.MavenCli.main(MavenCli.java:199)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced(Launcher.java:289)
	at org.codehaus.plexus.classworlds.launcher.Launcher.launch(Launcher.java:229)
	at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode(Launcher.java:415)
	at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:356)
Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed.
	at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:326)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134)
	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:207)
	... 20 more
Caused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1)
	at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404)
	at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166)
	at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:804)
	at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:751)
	at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:313)
	... 22 more
[ERROR] 
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
container ed0292f131d2004bc9d2aacd7fb7c95f0a589bee2d9b3b1e2f868823f92fc4d7 is dead
Thu Jul  4 03:48:24 CEST 2019

@victornoel
Copy link
Collaborator Author

@llorllale release failed...

@llorllale
Copy link
Contributor

@rultor release, tag is 0.42

@rultor
Copy link
Collaborator

rultor commented Jul 7, 2019

@rultor release, tag is 0.42

@llorllale OK, I will release it now. Please check the progress here

@rultor
Copy link
Collaborator

rultor commented Jul 7, 2019

@rultor release, tag is 0.42

@llorllale Done! FYI, the full log is here (took me 13min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants