openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

See below

If we need to keep adding str to things should Application methods be internally wrapped with str not adding str all over the code.

Diff comments:

> === modified file 'openlp/core/__init__.py'
> --- openlp/core/__init__.py	2017-05-30 18:42:35 +0000
> +++ openlp/core/__init__.py	2017-08-02 06:25:28 +0000
> @@ -181,7 +181,7 @@
>          """
>          Check if the data folder path exists.
>          """
> -        data_folder_path = AppLocation.get_data_path()
> +        data_folder_path = str(AppLocation.get_data_path())

Why do we have to add back in the str this harks back to the bad old days

>          if not os.path.exists(data_folder_path):
>              log.critical('Database was not found in: ' + data_folder_path)
>              status = QtWidgets.QMessageBox.critical(None, translate('OpenLP', 'Data Directory Error'),
> 
> === modified file 'openlp/core/common/applocation.py'
> --- openlp/core/common/applocation.py	2016-12-31 11:01:36 +0000
> +++ openlp/core/common/applocation.py	2017-08-02 06:25:28 +0000
> @@ -84,84 +87,97 @@
>      def get_data_path():
>          """
>          Return the path OpenLP stores all its data under.
> +
> +        :return: The data path to use.
> +        :rtype: pathlib.Path
>          """
>          # Check if we have a different data location.
>          if Settings().contains('advanced/data path'):
> -            path = Settings().value('advanced/data path')
> +            path = Path(Settings().value('advanced/data path'))
>          else:
>              path = AppLocation.get_directory(AppLocation.DataDir)
> -            check_directory_exists(path)
> -        return os.path.normpath(path)
> +            check_directory_exists(str(path))
> +        return path
>  
>      @staticmethod
> -    def get_files(section=None, extension=None):
> +    def get_files(section=None, extension=''):
>          """
>          Get a list of files from the data files path.
>  
> -        :param section: Defaults to *None*. The section of code getting the files - used to load from a section's
> -            data subdirectory.
> -        :param extension:
> -            Defaults to *None*. The extension to search for. For example::
> -
> -                '.png'
> +        :param section: Defaults to *None*. The section of code getting the files - used to load from a section's data
> +        subdirectory.
> +        :type section: None | str
> +
> +        :param extension: Defaults to ''. The extension to search for. For example::
> +            '.png'
> +        :type extension: str
> +
> +        :return: List of files found.
> +        :rtype: list[pathlib.Path]
>          """
>          path = AppLocation.get_data_path()
>          if section:
> -            path = os.path.join(path, section)
> +            path = path / section

is this right

>          try:
> -            files = os.listdir(path)
> +            file_paths = path.glob('*' + extension)
> +            return [file_path.relative_to(path) for file_path in file_paths]
>          except OSError:
>              return []
> -        if extension:
> -            return [filename for filename in files if extension == os.path.splitext(filename)[1]]
> -        else:
> -            # no filtering required
> -            return files
>  
>      @staticmethod
>      def get_section_data_path(section):
>          """
>          Return the path a particular module stores its data under.
> +
> +        :type section: str
> +
> +        :rtype: pathlib.Path
>          """
> -        data_path = AppLocation.get_data_path()
> -        path = os.path.join(data_path, section)
> -        check_directory_exists(path)
> +        path = AppLocation.get_data_path() / section
> +        check_directory_exists(str(path))
>          return path
>  
>  
>  def _get_os_dir_path(dir_type):
>      """
>      Return a path based on which OS and environment we are running in.
> +
> +    :param dir_type: AppLocation Enum of the requested path type
> +    :type dir_type: AppLocation Enum
> +
> +    :return: The requested path
> +    :rtype: pathlib.Path
>      """
>      # If running from source, return the language directory from the source directory
>      if dir_type == AppLocation.LanguageDir:
> -        directory = os.path.abspath(os.path.join(os.path.dirname(openlp.__file__), '..', 'resources'))
> -        if os.path.exists(directory):
> +        directory = Path(os.path.abspath(os.path.join(os.path.dirname(openlp.__file__), '..', 'resources')))
> +        if directory.exists():
>              return directory
>      if is_win():
> +        openlp_folder_path = Path(os.getenv('APPDATA'), 'openlp')
>          if dir_type == AppLocation.DataDir:
> -            return os.path.join(str(os.getenv('APPDATA')), 'openlp', 'data')
> +            return openlp_folder_path / 'data'
>          elif dir_type == AppLocation.LanguageDir:
>              return os.path.dirname(openlp.__file__)
> -        return os.path.join(str(os.getenv('APPDATA')), 'openlp')
> +        return openlp_folder_path
>      elif is_macosx():
> +        openlp_folder_path = Path(os.getenv('HOME'), 'Library', 'Application Support', 'openlp')
>          if dir_type == AppLocation.DataDir:
> -            return os.path.join(str(os.getenv('HOME')), 'Library', 'Application Support', 'openlp', 'Data')
> +            return openlp_folder_path / 'Data'
>          elif dir_type == AppLocation.LanguageDir:
>              return os.path.dirname(openlp.__file__)
> -        return os.path.join(str(os.getenv('HOME')), 'Library', 'Application Support', 'openlp')
> +        return openlp_folder_path
>      else:
>          if dir_type == AppLocation.LanguageDir:
> -            for prefix in ['/usr/local', '/usr']:
> -                directory = os.path.join(prefix, 'share', 'openlp')
> -                if os.path.exists(directory):
> -                    return directory
> -            return os.path.join('/usr', 'share', 'openlp')
> +            directory = Path('/usr', 'local', 'share', 'openlp')
> +            if directory.exists():
> +                return directory
> +            return Path('/usr', 'share', 'openlp')
>          if XDG_BASE_AVAILABLE:
> -            if dir_type == AppLocation.DataDir:
> -                return os.path.join(str(BaseDirectory.xdg_data_home), 'openlp')
> +            if dir_type == AppLocation.DataDir or dir_type == AppLocation.CacheDir:
> +                return Path(BaseDirectory.xdg_data_home, 'openlp')
>              elif dir_type == AppLocation.CacheDir:
> -                return os.path.join(str(BaseDirectory.xdg_cache_home), 'openlp')
> +                return Path(BaseDirectory.xdg_cache_home, 'openlp')
>          if dir_type == AppLocation.DataDir:
> -            return os.path.join(str(os.getenv('HOME')), '.openlp', 'data')
> -        return os.path.join(str(os.getenv('HOME')), '.openlp')
> +            return Path(os.getenv('HOME'), '.openlp', 'data')
> +        return Path(os.getenv('HOME'), '.openlp')


-- 
https://code.launchpad.net/~phill-ridout/openlp/pathlib1/+merge/328435
Your team OpenLP Core is subscribed to branch lp:openlp.