Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.5.0-beta-1
    • Fix Version/s: 3.5.2
    • Component/s: Embedding
    • Labels:
      None

      Description

      There seems to be some old unnecessary code in the MavenCli.java
      that could be cleaned up.

        Issue Links

          Activity

          Hide
          khmarbaise Karl Heinz Marbaise added a comment -

          Why not posting this code snippets into the description instead of attaching screen shots? Apart from that maybe I misunderstand a thing here? Which code is unnecessary?

          Show
          khmarbaise Karl Heinz Marbaise added a comment - Why not posting this code snippets into the description instead of attaching screen shots? Apart from that maybe I misunderstand a thing here? Which code is unnecessary?
          Hide
          stefan.eicher@gmail.com Stefan Eicher added a comment - - edited

          Incredible how fast you were! I was just interrupted by my wife and could not finish the issue. I posted a printscreen that you can see the code line...

          I would propose to remove this part since userToolchainsFile is never used:

          File userToolchainsFile;
                  if ( commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) )
                  {
                      userToolchainsFile = new File( commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) );
                      userToolchainsFile = resolveFile( userToolchainsFile, workingDirectory );
                  }
                  else
                  {
                      userToolchainsFile = MavenCli.DEFAULT_USER_TOOLCHAINS_FILE;
                  }
          

          It seems as this code once has been moved to here:

           private void toolchains( CliRequest cliRequest )
                  throws Exception
              {
                  File userToolchainsFile;
          
                  if ( cliRequest.commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) )
                  {
                      userToolchainsFile =
                          new File( cliRequest.commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) );
                      userToolchainsFile = resolveFile( userToolchainsFile, cliRequest.workingDirectory );
          
                      if ( !userToolchainsFile.isFile() )
                      {
                          throw new FileNotFoundException(
                              "The specified user toolchains file does not exist: " + userToolchainsFile );
                      }
                  }
          

          Gruss Stefan

          Show
          stefan.eicher@gmail.com Stefan Eicher added a comment - - edited Incredible how fast you were! I was just interrupted by my wife and could not finish the issue. I posted a printscreen that you can see the code line... I would propose to remove this part since userToolchainsFile is never used: File userToolchainsFile; if ( commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ) { userToolchainsFile = new File( commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ); userToolchainsFile = resolveFile( userToolchainsFile, workingDirectory ); } else { userToolchainsFile = MavenCli.DEFAULT_USER_TOOLCHAINS_FILE; } It seems as this code once has been moved to here: private void toolchains( CliRequest cliRequest ) throws Exception { File userToolchainsFile; if ( cliRequest.commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ) { userToolchainsFile = new File( cliRequest.commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ); userToolchainsFile = resolveFile( userToolchainsFile, cliRequest.workingDirectory ); if ( !userToolchainsFile.isFile() ) { throw new FileNotFoundException( "The specified user toolchains file does not exist: " + userToolchainsFile ); } } Gruss Stefan
          Hide
          khmarbaise Karl Heinz Marbaise added a comment -

          I was just that fast cause I was at my computer at this moment.....furthermore if you take a look some lines down you will see that userToolchainsFile is used:

                  File userToolchainsFile;
          
                  if ( cliRequest.commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) )
                  {
                      userToolchainsFile =
                          new File( cliRequest.commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) );
                      userToolchainsFile = resolveFile( userToolchainsFile, cliRequest.workingDirectory );
          
                      if ( !userToolchainsFile.isFile() )
                      {
                          throw new FileNotFoundException(
                              "The specified user toolchains file does not exist: " + userToolchainsFile );
                      }
                  }
                  else
                  {
                      userToolchainsFile = DEFAULT_USER_TOOLCHAINS_FILE;
                  }
          
                  File globalToolchainsFile;
          
                  if ( cliRequest.commandLine.hasOption( CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS ) )
                  {
                      globalToolchainsFile =
                          new File( cliRequest.commandLine.getOptionValue( CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS ) );
                      globalToolchainsFile = resolveFile( globalToolchainsFile, cliRequest.workingDirectory );
          
                      if ( !globalToolchainsFile.isFile() )
                      {
                          throw new FileNotFoundException(
                              "The specified global toolchains file does not exist: " + globalToolchainsFile );
                      }
                  }
                  else
                  {
                      globalToolchainsFile = DEFAULT_GLOBAL_TOOLCHAINS_FILE;
                  }
          
                  cliRequest.request.setGlobalToolchainsFile( globalToolchainsFile );
                  cliRequest.request.setUserToolchainsFile( userToolchainsFile );
          

          Kind regards
          Karl Heinz

          Show
          khmarbaise Karl Heinz Marbaise added a comment - I was just that fast cause I was at my computer at this moment.....furthermore if you take a look some lines down you will see that userToolchainsFile is used: File userToolchainsFile; if ( cliRequest.commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ) { userToolchainsFile = new File( cliRequest.commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ); userToolchainsFile = resolveFile( userToolchainsFile, cliRequest.workingDirectory ); if ( !userToolchainsFile.isFile() ) { throw new FileNotFoundException( "The specified user toolchains file does not exist: " + userToolchainsFile ); } } else { userToolchainsFile = DEFAULT_USER_TOOLCHAINS_FILE; } File globalToolchainsFile; if ( cliRequest.commandLine.hasOption( CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS ) ) { globalToolchainsFile = new File( cliRequest.commandLine.getOptionValue( CLIManager.ALTERNATE_GLOBAL_TOOLCHAINS ) ); globalToolchainsFile = resolveFile( globalToolchainsFile, cliRequest.workingDirectory ); if ( !globalToolchainsFile.isFile() ) { throw new FileNotFoundException( "The specified global toolchains file does not exist: " + globalToolchainsFile ); } } else { globalToolchainsFile = DEFAULT_GLOBAL_TOOLCHAINS_FILE; } cliRequest.request.setGlobalToolchainsFile( globalToolchainsFile ); cliRequest.request.setUserToolchainsFile( userToolchainsFile ); Kind regards Karl Heinz
          Hide
          stefan.eicher@gmail.com Stefan Eicher added a comment -

          I was talking of the File userToolchainsFile; in line 1465 not the one in 1215

          Show
          stefan.eicher@gmail.com Stefan Eicher added a comment - I was talking of the File userToolchainsFile; in line 1465 not the one in 1215
          Hide
          khmarbaise Karl Heinz Marbaise added a comment - - edited

          Ah Ok...wrong location....Now I understand your comments...oh my god...I have mistaken this sorry...

          Show
          khmarbaise Karl Heinz Marbaise added a comment - - edited Ah Ok...wrong location....Now I understand your comments...oh my god...I have mistaken this sorry...
          Hide
          khmarbaise Karl Heinz Marbaise added a comment - - edited

          Hm..

              private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request )
              {
                 .
                 .
                 .
                  String alternatePomFile = null;
                  if ( commandLine.hasOption( CLIManager.ALTERNATE_POM_FILE ) )
                  {
                      alternatePomFile = commandLine.getOptionValue( CLIManager.ALTERNATE_POM_FILE );
                  }
          
                  File userToolchainsFile;
                  if ( commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) )
                  {
                      userToolchainsFile = new File( commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) );
                      userToolchainsFile = resolveFile( userToolchainsFile, workingDirectory );
                  }
                  else
                  {
                      userToolchainsFile = MavenCli.DEFAULT_USER_TOOLCHAINS_FILE;
                  }
          ...
          

          This really looks like unnecessary code...cause it's not referenced anywhere else in this method...

          Show
          khmarbaise Karl Heinz Marbaise added a comment - - edited Hm.. private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request ) { . . . String alternatePomFile = null ; if ( commandLine.hasOption( CLIManager.ALTERNATE_POM_FILE ) ) { alternatePomFile = commandLine.getOptionValue( CLIManager.ALTERNATE_POM_FILE ); } File userToolchainsFile; if ( commandLine.hasOption( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ) { userToolchainsFile = new File( commandLine.getOptionValue( CLIManager.ALTERNATE_USER_TOOLCHAINS ) ); userToolchainsFile = resolveFile( userToolchainsFile, workingDirectory ); } else { userToolchainsFile = MavenCli.DEFAULT_USER_TOOLCHAINS_FILE; } ... This really looks like unnecessary code...cause it's not referenced anywhere else in this method...
          Hide
          rfscholte Robert Scholte added a comment -

          It should be in sync with the way settings are handled. I can imagine that I missed something when introducing the global toolchains.

          Show
          rfscholte Robert Scholte added a comment - It should be in sync with the way settings are handled. I can imagine that I missed something when introducing the global toolchains.
          Hide
          khmarbaise Karl Heinz Marbaise added a comment -

          That's not a problem...only those lines mentioned from #1465 to #1474 are simply not used anywhere else in this method and do not have a reference...so we can remove them...

          Show
          khmarbaise Karl Heinz Marbaise added a comment - That's not a problem...only those lines mentioned from #1465 to #1474 are simply not used anywhere else in this method and do not have a reference...so we can remove them...
          Hide
          khmarbaise Karl Heinz Marbaise added a comment -

          Stefan Eicher Would you like to offer a pull request ?

          Show
          khmarbaise Karl Heinz Marbaise added a comment - Stefan Eicher Would you like to offer a pull request ?
          Hide
          stefan.eicher@gmail.com Stefan Eicher added a comment - - edited

          ok I will do it tomorrow

          Show
          stefan.eicher@gmail.com Stefan Eicher added a comment - - edited ok I will do it tomorrow
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user stefaneicher opened a pull request:

          https://github.com/apache/maven/pull/111

          MNG-6203 Minor cleanup in MavenCli.java

          There is some unnecessary code in the MavenCli.java from line #1465 to #1474.
          The functionality has been moved to line #1215.

          Signed-off-by: Stefan Eicher <stefan.eicher@gmail.com>

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/stefaneicher/maven MNG-6203

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/maven/pull/111.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #111


          commit d73d36fc82babd52800ab25e537a9b973ee51c68
          Author: Stefan Eicher <stefan.eicher@gmail.com>
          Date: 2017-04-05T18:52:01Z

          MNG-6203 Minor cleanup in MavenCli.java

          There is some unnecessary code in the MavenCli.java from line #1465 to #1474.
          The functionality has been moved to line #1215.

          Signed-off-by: Stefan Eicher <stefan.eicher@gmail.com>


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user stefaneicher opened a pull request: https://github.com/apache/maven/pull/111 MNG-6203 Minor cleanup in MavenCli.java There is some unnecessary code in the MavenCli.java from line #1465 to #1474. The functionality has been moved to line #1215. Signed-off-by: Stefan Eicher <stefan.eicher@gmail.com> You can merge this pull request into a Git repository by running: $ git pull https://github.com/stefaneicher/maven MNG-6203 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/maven/pull/111.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #111 commit d73d36fc82babd52800ab25e537a9b973ee51c68 Author: Stefan Eicher <stefan.eicher@gmail.com> Date: 2017-04-05T18:52:01Z MNG-6203 Minor cleanup in MavenCli.java There is some unnecessary code in the MavenCli.java from line #1465 to #1474. The functionality has been moved to line #1215. Signed-off-by: Stefan Eicher <stefan.eicher@gmail.com>
          Hide
          stefan.eicher@gmail.com Stefan Eicher added a comment -
          Show
          stefan.eicher@gmail.com Stefan Eicher added a comment - done -> https://github.com/apache/maven/pull/111 Grüsse Stefan
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on the issue:

          https://github.com/apache/maven/pull/111

          If you add the JIRA ticket this PR is automatically added to the JIRA ticket...Thanks for this..

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on the issue: https://github.com/apache/maven/pull/111 If you add the JIRA ticket this PR is automatically added to the JIRA ticket...Thanks for this..
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on the issue:

          https://github.com/apache/maven/pull/111

          This pull request has been integrated into maven core. Thanks for your support. Please close this pull request.

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on the issue: https://github.com/apache/maven/pull/111 This pull request has been integrated into maven core. Thanks for your support. Please close this pull request.
          Hide
          khmarbaise Karl Heinz Marbaise added a comment -

          Thanks Stefan for your support.
          Done with 4c6d3a3462783507921d1a0100e3fa22ef8a98e4

          Show
          khmarbaise Karl Heinz Marbaise added a comment - Thanks Stefan for your support. Done with 4c6d3a3462783507921d1a0100e3fa22ef8a98e4
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user stefaneicher closed the pull request at:

          https://github.com/apache/maven/pull/111

          Show
          githubbot ASF GitHub Bot added a comment - Github user stefaneicher closed the pull request at: https://github.com/apache/maven/pull/111
          Hide
          stefan.eicher@gmail.com Stefan Eicher added a comment -

          Hi Karl Heinz,

          Your welcome. Thanks for your support. I just wanted to dig a bit deeper in maven and start reading the code. Thereby I thought if i do so, why not contribute even if it is just something that trivial. I was surprised by the attention I reveived. ... keeps me motivated.

          Gruss Stefan

          Show
          stefan.eicher@gmail.com Stefan Eicher added a comment - Hi Karl Heinz, Your welcome. Thanks for your support. I just wanted to dig a bit deeper in maven and start reading the code. Thereby I thought if i do so, why not contribute even if it is just something that trivial. I was surprised by the attention I reveived. ... keeps me motivated. Gruss Stefan

            People

            • Assignee:
              khmarbaise Karl Heinz Marbaise
              Reporter:
              stefan.eicher@gmail.com Stefan Eicher
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development