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

Revision history for this message
Данило Шеган (danilo) wrote :

Thanks, tests look much neater now. Thanks for the code changes as well, r=me.

As for your comment about getcwd stuff, there was a simple way around it:

  class LinaroIncludeReplacer(object):
      base_path = None

      def read_file_with_include_data(self, matchobj):
          # Assume self.base_path is the allowed path, instead of getcwd().
          ...

  def _process_include_tags(contents, base_path):
      replacer = LinaroIncludeReplacer()
      replacer.base_path = base_path

      contents = LINARO_INCLUDE_FILE_RE.sub(replacer.read_file_with_include_data, contents)
      ...

Anyway, no need to go to these lengths now (there are shorter but less readable tricks involving lambdas as well :), it's looking good, so let's get it landed and email Fathi/Alex when it's up on the server.

PS. You don't have to check for os.path.islink() anymore: the normalization/realpath code resolves all the links.

review: Approve

« Back to merge proposal