From: Jakub Narebski Date: Thu, 03 Jun 2010 18:43:13 GMT Subject: Re: [PATCH GSoC 1/3] gitweb: Create Gitweb::Config module Message-ID: <201006032043.14071.jnareb@gmail.com> In-Reply-To: Pavan Kumar Sunkara wrote: > 2010/6/3 Jakub Narebski : >> On Tue, 3 Jun 2010, Petr Baudis wrote: >>>> + >>>> +use Gitweb::Config; >>>> >>>>  BEGIN { >>>>     CGI->compile() if $ENV{'MOD_PERL'}; >>>>  } >>>> >>>> -our $version = "++GIT_VERSION++"; >>>> +$version = "++GIT_VERSION++"; >> >> This change is not necessary. >> >>  our $version = "++GIT_VERSION++"; >> >> would keep working even if '$version' is declared in other module and >> exported by this module (is imported into current scope). > > Ok. Will change it. [...] >> I would say >> >>        our $GIT = "++GIT_BINDIR++/git"; > > But, I think when we start reading the code, it would seem that 'our > $GIT' implies that it is a variable created locally rather than an > exported variable from Gitweb::Config module. From `perldoc -f our`: An "our" declares the listed variables to be valid globals within the enclosing block, file, or "eval". That is, it has the same scoping rules as a "my" declaration, but does not create a local variable. [...] The package in which the variable is entered is determined at the point of the declaration, [...] So if the variable already exists in given scope, for example if the variable was imported from other package, the 'our' declaration would be a no-op. > > Even though it increases the patch size, I don't think it will be much > of a concern when it comes to good redability of code. > > Jakub: Can you reply, what you think about this argument ? But I agree that first, 'our $var' seems to imply that it is _new_ variable declared in current scope, and second if we make a typo in variable name it wouldn't be detected as different from exported variable: 'our' will create new variable. So I agree that removing 'our' is a good idea, especially together with putting all variables that should be there in Gitweb::Config together with comments, even if they are configured during build process. Perhaps those declarations in Gitweb::Config should have in-line comment that they are defined in gitweb.cgi / gitweb.perl? -- Jakub Narebski Poland