mvhub-dev team mailing list archive

[Dan MacNeil <dan@xxxxxxxxxx>]

Review: Approve
This is a huge diff to look at, much of it is noise. All the files with headers that have been added should not be part of this merge request.  If the header is wrong (which it is), you will have to edit all the files by hand to fix it.

You should create a new branch with only the header adding script.  **NOT** the files that have changed. 

0) create new branch
1)  Create/Edit your script 
2) commit your script
3) run your script to test
4) examine output
5) bzr revert  # this will 
5) submit merge request

#########

There is no reason to have each key word on a serperate line and no reason to have that big if block in your main code. Just define $doc_header with the keywords and pre-pend that. 

 google: perl heredoc

##########
The only thing worse than no documention is bad documentation 

the header you add should not contain PURPOSE because that HEADER line should make sense and can only be added by hand. 

##########

If you run the script more than one, you will create multiple headers. 

The logic for this script should be:

if (has_no_header($file)){
    add_header($file);
} else{
   # header is there, nothing need be done
   # 

} 

########
You are not appending (putting something at the end) you are prepending (putting something at the start) 

########
branch names should be in all small letters. 

   lp:~himabindu-sanagavarapu/mvhub/ADD_HEADER_TO_MISSING_FILES

no need to fix now, but use small letters in the future (like when you create the new branch above)

############

mv_prove doesn't run, therefore the tests all *****FAIL***** 

You ****MUST*** run the tests and they must ***PASS*** before you sumbit a merge request. 

##########
The reason they are failing ***AGAIN*** is you've removed the executable bit from all the scripts in app-mvhub/bin/

I'd guess is that you are moving files back and forth from a windows machine, which is fine as long as you don't change things that shouldn't be changed. 

cdw is also broken because mv_get_active is no longer executable 

##########
The directory 

  app-mvhub/bin 

...is for files that are part of the application, that the end user will use

The directory:

   app-mvhub/project-tools/bin 

...is for tools we use to develop the application.

Our development tools are prefixed "mv_"

Therefore 

   app-mvhub/bin/header_add_v1.pl 

should be in:

   app-mvhub/projet-tools/bin/mv_add_doc_header



-- 
https://code.launchpad.net/~himabindu-sanagavarapu/mvhub/ADD_HEADER_TO_MISSING_FILES/+merge/33944
Your team MVHub Developers is subscribed to branch lp:mvhub.