Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace paths with resources #281

Merged
merged 12 commits into from
Nov 5, 2020
Merged

Replace paths with resources #281

merged 12 commits into from
Nov 5, 2020

Conversation

gnarlyquack
Copy link

Description

Moving gourmet into a src directory broke the "About" and "Help" dialogs, since they used file system paths to access the LICENSE and FAQ documents and made assumptions about how gourmet is installed (specifically, that it's a source install that mirrored the previous file system structure).

This commit replaces the doc_base global variable with package resources and fixes the broken "About" and "Help" dialogs. I'll be adding further commits to this PR that replaces the remaining paths with resources. This will ensure that all resources are bundled with gourmet regardless of how it's distributed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@gnarlyquack gnarlyquack changed the title Replaces paths with resources Replace paths with resources Nov 2, 2020
Copy link
Collaborator

@cydanil cydanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's nice

src/gourmet/gtk_extras/dialog_extras.py Outdated Show resolved Hide resolved
src/gourmet/gtk_extras/dialog_extras.py Show resolved Hide resolved
@@ -1,5 +1,6 @@
import os
import os.path
from pkgutil import get_data as _get_data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do

Suggested change
from pkgutil import get_data as _get_data
from pkgutil import get_data

I remember your point of hiding the imports on import, yet it's not quite idiomatic Python.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would disagree. It may not be common, but in addition to being specifically mentioned in PEP 8, it's used in Python's own standard library:
https://stackoverflow.com/questions/47138594/why-are-python-imports-renamed-with-leading-underscores
https://stackoverflow.com/questions/20757763/why-are-modules-imported-as-name-in-another-module

But besides just stylistic issues, there's an important pragmatic reason as underscored in PEP 8:

Imported names should always be considered an implementation detail. Other modules must not rely on indirect access to such imported names unless they are an explicitly documented part of the containing module's API

Note that imported names become part of a module's symbol table just like any other name, so from foo import * or from foo import get_data will work identically as if get_data was defined in the module itself. The best way to enforce this is by prefixing them with an underscore. While __all__ provides partial mitigation in cases of import *, it doesn't guard against explicit imports:

from foo import get_data  # this must be ok?
from foo import _get_data  # this looks like a problem

And as noted in the PEP:

Even with __all__ set appropriately, internal interfaces ... should still be prefixed with a single leading underscore.

src/gourmet/optionparser.py Show resolved Hide resolved
@@ -87,6 +88,12 @@ def image_to_bytes(image: Image.Image) -> bytes:
return ofi.getvalue()


def load_pixbuf_from_resource(resource_name: str) -> Pixbuf:
data = _get_data('gourmet', f'data/images/{resource_name}')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
data = _get_data('gourmet', f'data/images/{resource_name}')
data = get_data(op.join('gourmet',' data', 'images', resource_name)

Don't forget to add a test, however simple :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be the final API, as I'm considering consolidating resource management into its down module.

@gnarlyquack
Copy link
Author

This pretty much finishes up removing the global directory variables. If it looks ok, I'll probably rebase this PR off master and make some additional tweaks (namely, adding some typehints) to address some of your comments above.

A couple takeaways I've come across which we probably want to open tickets for:

  • I think we'll want to re-architect plugin discovery using namespace packages instead of crawling file paths. I have not spent much time thinking about it or looking into the plugin system, but this would seem to simplify the process quite a bit and remove the need for .gourmet-plugin files. Moreover, as I noted in the code itself, I doubt the current system is portable and will fail to discover bundled plugins if gourmet is being run as a built distribution (e.g., a wheel) since there's no file path. Has anybody actually tested the wheel recently?
  • It doesn't look like the .mo translation files are actually being used anywhere. It looks like localization needs to initialized, and in order to make the locale directory portable, my inclination is to move the .mo files and the po directory back into gourmet as package data. Although, again, I don't know if this works for non-source distributions. This solution on Stack Overflow uses setuptools.pkg_resources to extract a file path from bundled package data, but I'd very much like to avoid a run-time dependency on setuptools. Again, I really haven't spent any time poking around with this, and given I'm a native english speaker, I don't even know how well I'd be able to test it.

This copies the FAQ and LICENSE into the package so they are available
as a resource.
This option and setting appear to been removed since at least commit
648a40f in October 2008.
@cydanil
Copy link
Collaborator

cydanil commented Nov 4, 2020

Yup, I'm generally happy with the improvements brought here.

Regarding plugins, I agree that they require some work. It's been on the back of my mind to rework them to use entry points, something like them being their own packages, with something like the following in their setup.py:

def _get_version_string():
    try:
        from gourmet.something_something import get_package_version
    except ImportError:
        print("WARNING: Gourmet version not found! Version will be blank!")
        return ''

    return get_package_version(dirname(__file__))

setup(name='MyGourmetPlugin',
      version=_get_version_string(),
      author='Cyril Danilevski',
      author_email='cydanil',
      description='A Gourmet Plugin to import whatever website',
      long_description='',
      url='',
      package_dir={'': 'src'},
      packages=find_packages('src'),
      entry_points={
          'gourmet.plugins': [
              'MyGourmetPlugin = MyGourmetPlugin.MyGourmetPlugin:PluginClassName'
          ],
      },
      package_data={},
      requires=[],
      )

Then, in Gourmet, something like the following could be done:

import pkg_resources
plugins = list(pkg_resources.iter_entry_points('gourmet.plugins'))

This would also ease 3rd party plugin development, where in Gourmet we could have a command like gourmet-plugin new MyGourmetPlugin which would create a folder with the correct skeleton and template files.

@gnarlyquack
Copy link
Author

The build appears to be failing because an issue with GitHub's build system, not because of anything with the PR itself:

Reading package lists...
E: Failed to fetch https://packagecloud.io/github/git-lfs/ubuntu/dists/focal/InRelease  402  Payment Required [IP: 54.183.38.243 443]
E: The repository 'https://packagecloud.io/github/git-lfs/ubuntu focal InRelease' is no longer signed.
Error: Process completed with exit code 100.

Anyway, this PR should be ready to go.

@gnarlyquack gnarlyquack marked this pull request as ready for review November 5, 2020 02:16
@gnarlyquack
Copy link
Author

Regarding plugins, I agree that they require some work. It's been on the back of my mind to rework them to use entry points, something like them being their own packages, with something like the following in their setup.py:

Yes, this method is also covered in the document I linked to in my previous comment. Again, without having looked too closely at any of it, using namespace packages seems to offer identical functionality using only Python's standard library and without having to add an additional runtime dependency on setuptools.

src/gourmet/timer.py Outdated Show resolved Hide resolved
@cydanil cydanil merged commit c884791 into kirienko:master Nov 5, 2020
@cydanil
Copy link
Collaborator

cydanil commented Nov 5, 2020

Thank you for this PR! It's nice work that helps make Gourmet better.

Now, we're not too far from a next release of Gourmet: all plugins work, and it's generally usable.

@gnarlyquack
Copy link
Author

You're welcome!

In doing this work, it appears the nutrition plugin isn't working, so I've started some work on that. I can open a PR for tracking purposes, if you'd like, but it's still work-in-progress. Even if I get it working as it before, there are some bugs I've come across that I'll probably end up opening tickets for.

I also think we can do away with the web plugin, which appears to be a plugin to run Gourmet as a web app using Django.

@gnarlyquack gnarlyquack deleted the paths-to-resources branch November 5, 2020 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants