Code review comment for lp:~gesha/linaro-license-protection/include-readme

Danilo,
>
> It would be good to split this up into several functions, each one
> testing a single corner case/condition (like you do now, but just not in
> a single method).
>

done

>
> If you just make the regular expression a compiled one and put it
> directly in views.py as eg. LINARO_INCLUDE_FILE_RE, you can import that,
> and then you'd at least be testing the right RE all the time (i.e. in
> this way, we can change the regular expression in the code, code would
> stop working, but tests would still pass).
>

I thought about it :)

> > +def repl(m):
> > + """
> > + Return content of file in current directory otherwise empty string.
> > + """
>
> Name and docstring are not very informative. How about
>
> def process_include_and_read_file()
>
> (That immediately tells you how method actually does two separate
> things, and should probably be split into two)

This function is only for re.sub() it returns a replacement string and takes only one parameter - match object.
Anyway I've updated name and docstring.

>
> > + content = ''
> > + fname = m.group('file_name')
> > + dirname = os.path.dirname(fname)
> > + if (not dirname or dirname=='.') and os.path.isfile(fname) and not
> os.path.islink(fname):
> > + with open(fname, "r") as infile:
> > + content = infile.read()
> > +
> > + return content
>
> Is it not simpler to check:
>
> full_filename = os.path.join(current_dir, fname)
> normalized_path = os.path.normpath(os.path.realname(full_filename))
> if current_dir == os.path.dirname(normalized_path):
> ....
>
> I'd also split this into a separate method "is_same_parent_dir(parent,
> filename)".
>

Implemented

>
> I generally hate this: we should not depend on cwd in our code, but
> instead pass appropriate parameters (path) around. I know it makes it
> harder for you since you can't simply pass the "repl" as the function to
> run, but it shouldn't be much harder :)
>

I agree, but in our case I don't see any other proper way (may be just now) :)