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

From: Jakub Narebski <[email-masked]>
Date: Sun, 18 Jul 2010 17:10:22 +0200

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.

> 
> 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.

> 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 <[email-masked]>

I have not checked the diff itself...

-- 
Jakub Narebski
Poland