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

Task 90, updated code #239

Closed
wants to merge 46 commits into from
Closed

Task 90, updated code #239

wants to merge 46 commits into from

Conversation

mentiflectax
Copy link
Contributor

Second pull request for fixing the issue #90 .

Notes:

  1. Please not that in order to build the project, I need to use the 1.0-SNAPSHOT version of the jcabi-ssl-maven-plugin until the issue jcabi-ssl-maven-plugin build fails with 3 test errors jcabi/jcabi-ssl-maven-plugin#4 is fixed. I created a pull request with changes that will fix it.

  2. I'm getting lots of code quality errors like shown below, which I'm fixing now.

[ERROR] \src\main\java\com\s3auth\hosts\DefaultDynamo.java[30]: Lines in file sh
ould end with Unix-like end of line (RegexpMultilineCheck)
[ERROR] \src\main\java\com\s3auth\hosts\DefaultDynamo.java[55]: Lines in file sh
ould end with Unix-like end of line (RegexpMultilineCheck)
[ERROR] \src\main\java\com\s3auth\hosts\DefaultDynamo.java[71]: Lines in file sh
ould end with Unix-like end of line (RegexpMultilineCheck)
  1. The tests seem to run.

…\DefaultDynamo.java[30]: Lines in file should end with Unix-like end of line (RegexpMultilineCheck).
…\DefaultDynamo.java[30]: Lines in file should end with Unix-like end of line (RegexpMultilineCheck).
…\DefaultDynamo.java[30]: Lines in file should end with Unix-like end of line (RegexpMultilineCheck).
* @version $Id$
*/
@Immutable
public interface RegionCreator {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Classes should be named as abstractions of real-life entities; i.e. by what they "are" not by what they "do". How about Regions? See http://www.yegor256.com/2014/04/27/typical-mistakes-in-java-code.html

@carlosmiranda
Copy link
Collaborator

@dpisarenko A few specific comments above. Furthermore, a couple of general comments:

  1. Don't ignore the static analysis errors, our merging bot won't accept any changes if there are warnings or errors: http://www.yegor256.com/2014/08/13/strict-code-quality-control.html#first-reaction
  2. The branch is too big. This may take some getting used to, but in PDD we don't necessarily finish everything in one go. You can choose to implement some stubs with corresponding ignored unit tests, and describe the remaining work in some puzzle comments. Keep things small and manageable. See http://www.yegor256.com/2009/03/04/pdd.html

@mentiflectax
Copy link
Contributor Author

@carlosmiranda Regarding line endings: git config --list says that core.autocrlf=true in my s3auth repository.

@mentiflectax
Copy link
Contributor Author

@carlosmiranda Now the buld fails with an error I don't know how to fix - Found duplicate classes/resources. If you know, how to diagnose/fix it, please tell me.

[ERROR] Failed to execute goal com.qulice:qulice-maven-plugin:0.12:check (jcabi-check) on project s3auth-hosts: Execution jcabi-check of goal com.qulice:qulice-maven-plugin:0.12:check failed: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal com.qulice:qulice-maven-plugin:0.12:check (jcabi-check) on project s3auth-hosts: Execution jcabi-check of goal com.qulice:qulice-maven-plugin:0.12:check failed: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:224)
    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:120)
    at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:347)
    at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:154)
    at org.apache.maven.cli.MavenCli.execute(MavenCli.java:582)
    at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:214)
    at org.apache.maven.cli.MavenCli.main(MavenCli.java:158)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    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.PluginExecutionException: Execution jcabi-check of goal com.qulice:qulice-maven-plugin:0.12:check failed: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:143)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208)
    ... 19 more
Caused by: java.lang.IllegalArgumentException: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
    at com.qulice.maven.MojoExecutor.execute(MojoExecutor.java:136)
    at com.qulice.maven.DuplicateFinderValidator.validate(DuplicateFinderValidator.java:91)
    at com.qulice.maven.CheckMojo.run(CheckMojo.java:86)
    at com.qulice.maven.CheckMojo.doExecute(CheckMojo.java:59)
    at com.qulice.maven.AbstractQuliceMojo.execute(AbstractQuliceMojo.java:175)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:132)
    ... 20 more
Caused by: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
    at com.ning.maven.plugins.duplicatefinder.DuplicateFinderMojo.checkClasspath(DuplicateFinderMojo.java:250)
    at com.ning.maven.plugins.duplicatefinder.DuplicateFinderMojo.checkCompileClasspath(DuplicateFinderMojo.java:201)
    at com.ning.maven.plugins.duplicatefinder.DuplicateFinderMojo.execute(DuplicateFinderMojo.java:178)
    at com.qulice.maven.MojoExecutor.execute(MojoExecutor.java:134)
    ... 25 more

@mentiflectax
Copy link
Contributor Author

@carlosmiranda Note that the issue jcabi/jcabi-ssl-maven-plugin#4 I mentioned above is now fixed and the PR was merged. But I don't know, how to make sure that the s3auth build uses the latest version. I added an issue for that ( jcabi/jcabi-ssl-maven-plugin#12 ).

….qulice:qulice-maven-plugin:0.12:check (jcabi-check) on project s3auth-hosts: Execution jcabi-check of goal com.qulice:qulice-maven-plugin:0.12:check failed: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources -> [Help 1]

                                   org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal com.qulice:qulice-maven-plugin:0.12:check (jcabi-check) on project s3auth-hosts: Execution jcabi-check of goal com.qulice:qulice-maven-plugin:0.12:check failed: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
                                       at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:224)
                                       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:120)
                                       at org.apache.maven.DefaultMaven.doExecute(DefaultMaven.java:347)
                                       at org.apache.maven.DefaultMaven.execute(DefaultMaven.java:154)
                                       at org.apache.maven.cli.MavenCli.execute(MavenCli.java:582)
                                       at org.apache.maven.cli.MavenCli.doMain(MavenCli.java:214)
                                       at org.apache.maven.cli.MavenCli.main(MavenCli.java:158)
                                       at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
                                       at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
                                       at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
                                       at java.lang.reflect.Method.invoke(Method.java:606)
                                       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.PluginExecutionException: Execution jcabi-check of goal com.qulice:qulice-maven-plugin:0.12:check failed: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
                                       at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:143)
                                       at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208)
                                       ... 19 more
                                   Caused by: java.lang.IllegalArgumentException: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
                                       at com.qulice.maven.MojoExecutor.execute(MojoExecutor.java:136)
                                       at com.qulice.maven.DuplicateFinderValidator.validate(DuplicateFinderValidator.java:91)
                                       at com.qulice.maven.CheckMojo.run(CheckMojo.java:86)
                                       at com.qulice.maven.CheckMojo.doExecute(CheckMojo.java:59)
                                       at com.qulice.maven.AbstractQuliceMojo.execute(AbstractQuliceMojo.java:175)
                                       at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:132)
                                       ... 20 more
                                   Caused by: org.apache.maven.plugin.MojoExecutionException: Found duplicate classes/resources
                                       at com.ning.maven.plugins.duplicatefinder.DuplicateFinderMojo.checkClasspath(DuplicateFinderMojo.java:250)
                                       at com.ning.maven.plugins.duplicatefinder.DuplicateFinderMojo.checkCompileClasspath(DuplicateFinderMojo.java:201)
                                       at com.ning.maven.plugins.duplicatefinder.DuplicateFinderMojo.execute(DuplicateFinderMojo.java:178)
                                       at com.qulice.maven.MojoExecutor.execute(MojoExecutor.java:134)
                                       ... 25 more
@carlosmiranda
Copy link
Collaborator

@dpisarenko Regarding the line endings, core.autocrlf must be set to false, and you need to do a clean checkout.

* @author Dmitri Pisarenko ([email protected])
* @version $Id$
*/
public class ReRegions implements Regions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't see the point of this class. Why not use com.jcabi.dynamo.Region instead of this class as a parameter DefaultDynamo's constructor and pass MkRegion to it in unit tests? http://dynamo.jcabi.com/example-mock.html

@carlosmiranda
Copy link
Collaborator

@dpisarenko Regarding your build failure, it would be better if you can raise a new ticket so that we can address it separately. Please see the following posts for guidance:

http://www.yegor256.com/2014/11/24/principles-of-bug-tracking.html
http://www.yegor256.com/2014/04/13/bugs-are-welcome.html

@carlosmiranda
Copy link
Collaborator

@dpisarenko Do we have any updates here?

@mentiflectax
Copy link
Contributor Author

@carlosmiranda No. On January 5th I decided to stop working on the open tasks (including this one), asked Yegor whether I have to finalize them and he said no (that I don't have to finalize it). So I didn't.

I think someone else has been assigned to the issue in question since then.

@carlosmiranda
Copy link
Collaborator

@dpisarenko No problem - can we close the pull request, then?

@davvd
Copy link

davvd commented Jan 14, 2015

@carlosmiranda I just added 27 mins to your account, many thanks for your contribution..

+27 to your rating, your total score is +968

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.

5 participants