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

Ensure all modules are properly loaded for "mix gettext.merge" #365

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

c4710n
Copy link
Contributor

@c4710n c4710n commented Jun 1, 2023

:nofile error of #358 has been solved for a while, but I found it's not solved completely.

I can still see the :nofile error when working with Gettext with custom plural module. I created a repo for reproducing the problem:

  • repo: https://github.com/plastic-gun/elixir-gettext-nofile
  • every commit in this repo is self-descriptive, you can track them to reproduce the problem.
  • README.md contains the following contents:
    • how to reproduce?
    • what is wrong?
    • the problem
    • the reason
    • the solution

I found the solution is very easy - use mix loadpaths, such as:

$ mix do loadpaths + gettext.merge priv/gettext --locale en_US

But, to minimize surprises, I believe it would be better to make the changes within Mix.Tasks.Gettext.Merge, which helps to reduce the friction for other developers. This PR does it.

I don't have a comprehensive understanding of mix's workings yet, so the solution in this PR maybe not perfect. Welcome to comment. ;)

And, I want to create a test for this case, but I don't know how. Sorry for that :(

@coveralls
Copy link

Pull Request Test Coverage Report for Build e71d886bcb0d9276df8d467fb2025640484e0e36-PR-365

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 90.801%

Totals Coverage Status
Change from base Build 8cc657d6704b420ffdb93c51b640bf9dc5d5614a: 0.06%
Covered Lines: 533
Relevant Lines: 587

💛 - Coveralls

@c4710n c4710n mentioned this pull request Jun 1, 2023
@maennchen maennchen changed the title Fix #358 again ensure all modules are properly loaded for gettext.merge mix task Jun 1, 2023
@maennchen
Copy link
Member

@c4710n Thanks!

The PR looks good to me, but @whatyouhide knows more about the internals around module loading I presume. So we should await his feedback.

@whatyouhide whatyouhide changed the title ensure all modules are properly loaded for gettext.merge mix task Ensure all modules are properly loaded for "mix gettext.merge" Jun 1, 2023
@c4710n
Copy link
Contributor Author

c4710n commented Jun 1, 2023

@whatyouhide New change's done.

@whatyouhide whatyouhide merged commit 0cfacae into elixir-gettext:main Jun 1, 2023
@whatyouhide
Copy link
Contributor

Thanks @c4710n! 💟

@c4710n c4710n deleted the fix-nofile branch June 1, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants