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

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

I have not checked the diff itself...

-- 
Jakub Narebski
Poland