Re: [PATCHv2 GSOC 08/11] gitweb: Create Gitweb::View module

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 18, 2010, 15:10 UTC
Message-ID: <201007181710.23105.jnareb@gmail.com>
In-Reply-To: <1279178951-23712-9-git-send-email-pavan.sss1991@gmail.com>

On Thu, 15 Jul 2010, Pavan Kumar Sunkara wrote:

> Create Gitweb::View module in 'gitweb/lib/Gitweb/View.pm'
> to store the subroutines related to the HTML output
> for gitweb.

If it contains subroutines related only to HTML output, why isn't it called Gitweb::HTML then? If it contains some subroutines which are not strictly about HTML, please state it in the commit message.

Show 5 quoted lines

> 
> This module depends on Git.pm, Config.pm, Request.pm,
> Escape.pm and RepoConfig.pm. Action specific HTML div
> subroutines are not included in this module due to
> unmet dependencies of Gitweb::Parse and Gitweb::Format.

Whether you use Gitweb::Git or Git.pm, you should be consistent in the naming you use: either Git.pm, or Gitweb::Parse.

Do I understand correctly trhat Gitweb::Parse and Gitweb::Format are to be added in subsequent patches? Please state this fact in the commit message.

Show 28 quoted lines

> 
> Subroutines moved:
> 	*href
> 	 get_feed_info
> 	*chop_str
> 	 chop_and_escape_str
> 	*age_class
> 	 age_string
> 	*S_ISGITLINK
> 	 mode_str
> 	 file_type
> 	 file_type_long
> 	*get_page_title
> 	 git_header_html
> 	 git_footer_html
> 	 die_error
> 	*git_print_page_nav
> 	 format_paging_nav
> 	 git_print_header_div
> 	 git_print_page_path
> 	*print_local_time
> 	 format_local_time
> 	*insert_file
> 	*git_get_link_target
> 	 normalize_link_target
> 	 git_print_tree_entry
> 	*print_sort_th
> 	 format_sort_th

Nitpick: my first thought was "why some of those subroutines are marked
with a star?".  If you want to divide this list of subroutines into
groups by function, you should leave a space between '*' and name of
subroutine (i.e. '* href'), and perhaps also leave empty lines between
groups, though this might make commit this list overly long.

I guess that this separation of subroutines into Gitweb::View module was a matter of some compromise: between having too large modules (containing only barely related subroutines) and having too many modules.

> 
> Update 'gitweb/Makefile' to install Gitweb::View alongside gitweb.
> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

I have not checked the diff itself...

-- 
Jakub Narebski
Poland