mvhub-dev team mailing list archive

[hsanagavarapu <hsanagav@xxxxxxxxxx>]

Review: Needs Information doubts
Sir,

Thank you for your detailed review.I have the following doubts in fixing my code.And before doing some changes I just want to make sure also.

> 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

> I will create a seperate branch with only header adding script.

> #########
> 
> 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
>  
I have a doubt here.The reason why I have created a seperate variable for each keyword is I think that some scripts may have only one or two keywords.So if script is having that keyword my script will add remaining keywords.
If every file will have all keywords or nothing at all,I can do $doc_header.please confirm about keywords in script files.
> ##########
> 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.

Should I remove PURPOSE keyword from each file which I have added?
> 
> ##########
> 
> 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
>    #
> 
> }
> In my script,I have the code for not adding keywords or headers multiple times.My script will make sure that.

if(!(_search_string_in_file($required_keyword, \@first_15_lines)))

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

> I din't understand your intention well.I am adding headers at the starting of file not at the end.Please clarify If I am wrong or not understood well.
> ########
> 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)
> I will take care about this from next time.
> ############
> 
> 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-commit/mvhub/trunk.