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

Pyinstaller hook #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Pyinstaller hook #41

wants to merge 2 commits into from

Conversation

alexitx
Copy link

@alexitx alexitx commented Jan 28, 2022

Added a hook for PyInstaller to package the dynamically imported locales (#40)

@laurentmmeyer
Copy link

Thank you very much for the PR. We reviewed it (@gr0uch0dev and I) and we have few questions:

  • Is there a way we can have your the pyinstaller locales list in one place instead of updating the README and the pyinstaller for every release? Like one single truth?
  • Can you provide the command to run the pyinstaller? We found pyinstaller -F -c --noupx -i None but we're not sure how it is supposed to play.

@alexitx
Copy link
Author

alexitx commented Jul 3, 2022

Is there a way we can have your the pyinstaller locales list in one place instead of updating the README and the pyinstaller for every release? Like one single truth?

There can be a globally defined constant containing all available locales which can even be used by the import mechanism in timeago.locales.timeago_template but the README still has to be updated manually. For example:

locale = 'en'
if locale in LOCALES:
    # Import the locale module and use it
else:
    # Fall back to default locale or raise exception

But after taking another look at the timeago.locales package, there is a lot that can be improved outside of this PR.

  • I don't think import_locale.py and convert_local_to_json.js belong in this package at all. Those modules don't seem to be used anywhere in this codebase and seems like something that should only be part of the tests.
  • create_tests.py should also be part of the tests and preferrably rewritten.
  • If all locale modules i.e. en.py are located in a separate package, then they can be iterated much easier with pkgutil.iter_modules(), dir() or module.__all__. Or better yet, the locale strings can reside as YAML or JSON files in a directory somewhere else in this repo and the locale Python modules can be generated dynamically by the CI before publishing the package.

This would be a major overhaul of the whole timeago library to make it easier to maintain. Not sure if everyone would agree with such changes.

Can you provide the command to run the pyinstaller? We found pyinstaller -F -c --noupx -i None but we're not sure how it is supposed to play.

I gave some sample code and the PyInstaller command in #40 which demonstrates the issue. With this PR it should print the properly formatted string because all the locales are known to PyInstaller, otherwise they can't be imported because they aren't bundled with the produced executable.

@lolobosse
Copy link
Collaborator

Hi @alexitx,

Thanks a lot for the exhaustive feedback, you're right, we would need a rewrite and a simplification.

I'll get on it as soon as I get time, I might write some specs at some point in some issues.

About PyInstaller, I think that I need to run it on a Windows machine as everything is working on my Mac

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.

3 participants