7
\$\begingroup\$

I wrote a perl script to import local files with "File-Ending-Versioning" (like file.1.12.pm) into a git repository. I tried to use the concept of roles in this script and I am not shure if I applied the concept right.

The basic idea is, that the script must be able to operate in a so-called "dryrun-mode": If invoked with the dryrun option, it should act as if it were doing an import like print log and error messages, but it should not actually create anything.

As the code is quite long, I shortened it to the relevant sections, but I am willing to expand if I missed something.

First, I have a main script file called git-importer.

#!/usr/bin/perl                                                                                                                                                                                                                                                                                                             

use strict;
use warnings;
use version; our $VERSION = qv( '0.3.0' );
use English qw( -no_match_vars );

use lib '/our/special/library/path';
use Utils::Cwd qw(abs_path); # oddity of my workplaces environment

# use lib './lib' but make it independant of the current directory
use File::Basename;
# move local lib folder to top of @INC
BEGIN { my $prog = unshift( @INC,dirname(abs_path($0))."/lib"); }

# those are the modules I use for my scripts behaviour
use Importer;
use Import::Git;
use Import::Git::dryrun;
use Import::Github;
use Import::Github::dryrun;
use Import::File;
use Import::File::dryrun;

use Getopt::Long::Descriptive;
use Getopt::Long::Descriptive::Opts;


my ( $opt, $usage ) = describe_options(
   "Import files into Github Repositories\n\n Usage: %c %o [FILE]",
   [ 'config|c=s',      "path to configuration file", { default => '/common/admin/etc/git-importer' } ],
   [ 'sourcefile|s=s@', "required - path to source file. If source is a softlink, this file will be taken as a starting version. ".
                        "Module names, like Utils::Test::... are supported if --modules is used", { required => 1 } ],
   [ 'targetdir|t=s',   "optional - path to target directory where local repository will be created" ],
   [ 'logfile|l=s',     "optional - path to logfile" ],
   [ 'remote|r=s',      "optional - ssh clone url of the remote repository" ],
   [ 'dryrun|d',        "optional - only perform read actions" ],
   [ 'modules|m     ',  "optional - Create a module repository", ],
);

my $importer = Importer->new();


my $params = $importer->init_config( $opt->config );
$importer->is_module($opt->modules);

# I did start a code review on a Perlmonks Meetup but we were not able to finish on time. They found it odd that I used a different object for the dryrun. 
$importer->check_permissions();
foreach my $file ( @{ $opt->sourcefile } ) { 
   $file = $importer->test_file( $file, $opt->modules );
   my $o_file;
   if ( $opt->dryrun ) { 
      $o_file = Import::File::dryrun->new( rawpath => $file, is_module => $opt->modules );
   }
   else {
      $o_file = Import::File->new( rawpath => $file, is_module => $opt->modules );
   }
   $importer->add_file( $o_file );
   $importer->add_type( $o_file->type );
}

[...]                                                                                                                                                                                                                                                                              

my $git;         
if ( $opt->dryrun ) { 
    $git = Import::Git::dryrun->new( git_import_user => $params->{ git_import_user }, targetdir => $params->{ targetdir }, tag => '1' );
}             
else {        
    $git = Import::Git->new( git_import_user => $params->{ git_import_user }, targetdir => $params->{ targetdir }, tag => '1' );
}   

[...]

My structure is as follows: I have a lib folder where I placed all my modules in like this:

.
├── Import
│   ├── File
│   │   ├── dryrun.pm
│   │   ├── Role.pm
│   │   ├── Version
│   │   │   └── dryrun.pm
│   │   └── Version.pm
│   ├── File.pm
│   ├── Git
│   │   ├── dryrun.pm
│   │   └── Role.pm
│   ├── Github
│   │   ├── dryrun.pm
│   │   └── Role.pm
│   ├── Github.pm
│   └── Git.pm
└── Importer.pm

So for all of my modules (except Importer.pm itself) there is a .pm, a dryrun.pm and a Role.pm.

I would like to look at Git.pm. The Role looks like this:

  #! /usr/bin/perl

  package Import::Git::Role;

  use strict;
  use warnings;

  use Mouse::Role;

  use Carp;
  use English qw( -no_match_vars );
  use File::Path;
  use Storable;
  use Utils::Log;

  # functionality that all classes using this Role must implement     
  requires qw(commit rmdir mkdir create_repository choose_repo_name push clone_empty_remote );  

  # attributes of all classes that use this role  
  has 'local'           => ( is => 'rw' );
  has 'remote'          => ( is => 'rw' );
  has 'targetdir'       => ( is => 'rw' );
  has 'msg_file'        => ( is => 'rw', trigger => \&read_msg_file );
  has 'messages'        => ( is => 'rw' );
  has 'author'          => ( is => 'rw' );
  has 'push_default'    => ( is => 'rw' );
  has 'git_import_user' => ( is => 'rw', trigger => \&build_author );
  has '_repo'           => ( is => 'rw' );
  has 'name'            => ( is => 'rw' );
  has 'tag'             => ( is => 'rw', default => '1' );

  sub rollback {
      my ( $self ) = @_; 
      $self->rmdir();

    return;
  }   

  sub choose_repo_name {
     my ( $self ) = @_; 

     print "\nPlease enter a name for your repository: \n";
     my $chosen_option = <STDIN>;
     chomp $chosen_option;

     while ( $chosen_option !~ /[A-Za-z0-9_-]+/xms ) { 
        print "Invalid characters in $chosen_option. Please try again\n";
        $chosen_option = <>; 
        chomp $chosen_option;
     }

     report LOG_INFO, "User chose $chosen_option as repository name\n";
     return $chosen_option;
  }   

  sub build_author {
     my ( $self, $import_user ) = @_; 

     $self->author( $import_user->{ name } . ' <' . $import_user->{ email } . '>' );
     $self->push_default( $import_user->{ push_default } );

    return;
  }   

It specifies what functionality must be implemented by the other classes and implements common functionality for Git.pm and dryrun.pm. This functionality is performed the same way, no matter if its a productive or a dryrun.

Then there is Git.pm, used when an actual import is performed: (only sub commit shown for brevity, there are more) #! /usr/bin/perl

  package Import::Git;

  use strict;
  use warnings;

  use Mouse;

  with 'Import::Git::Role';

  use Carp;
  use English qw( -no_match_vars );
  use File::Path qw(:DEFAULT make_path);
  use Git;

  use Utils::Log;

  sub commit {
     my ( $self, $name, $version ) = @_; 

     my $message = "$name Version $version\nImported from " . $name . " in version $version";

    if ( $self->messages && $self->messages->{ $name }->{ $version } ) { 
       $message = $self->messages->{ $name }->{ $version };  
     }

   my $rc = $self->_repo->command( 'add', '-A' );

     eval {
        $rc = $self->_repo->command( 'commit', '-m', $message, '--author', $self->author );
        1;  
     } or do {
       #Error 1 =  Nothing to commit. We can ignore this, its two versions of our file whichcontain no differences
       unless ( $EVAL_ERROR->{'-value'} == '1' ) {    
           die "Commit failed: $EVAL_ERROR";
        }   
     };  

     my $msg = "commited file $name in version $version";

      if ( $self->tag == 1 ) { 
          $rc = $self->_repo->command( "tag", "$version" );
          $msg = "commited and tagged file $name in version $version";
     }   
    return $msg;
  }
  [...]

And there is dryrun.pm for performing the testrun:

  #! /usr/bin/perl                                                                                                                                                                                                                                                                                                            

  package Import::Git::dryrun;

  use strict;
  use warnings;

  use Mouse;
  with 'Import::Git::Role';
  use Carp;
  use English qw( -no_match_vars );
  use File::Path;
  use Utils::Log;

  sub commit {
     my ( $self, $name, $version ) = @_; 

     my $message = "$name Version $version\nImported from " . $name . " in version $version";

     if ( $self->messages ) { 
        $message = $self->messages->{ $name }->{ $version } if $self->messages->{ $name }->{ $version };
     }

     print "git add -A\n";
     print "git commit -m $message --author " . $self->author . "\n";
     print "git tag $version\n" if $self->tag == 1;

    return "Commited file $name in Version $version";
  }

I wanted to use the Role to share behaviour that is the same between Git.pm and dryrun.pm and to mandate the list of functions that both of them would have to use. I am looking for feedback on my code quality, what I could improve and wheter this is a proper usage of Roles.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Sep 10 '18 at 10:20
  • \$\begingroup\$ Code looks pretty clean. However maybe you should include left out code ([...]) or, if you ran out of space, link to the repo. \$\endgroup\$ – yuri Sep 14 '18 at 6:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.