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

Add Afghanistan holidays #1144

Open
arkid15r opened this issue May 5, 2023 · 14 comments · May be fixed by #2198
Open

Add Afghanistan holidays #1144

arkid15r opened this issue May 5, 2023 · 14 comments · May be fixed by #2198

Comments

@arkid15r
Copy link
Collaborator

arkid15r commented May 5, 2023

Some links:

@Prateekshit73
Copy link

While running the tests, I encountered an assertion error indicating a mismatch between the expected and actual supported languages for Afghanistan. The test expects only ['fa'], but the implementation supports both ['fa', 'ps']. I want to ensure that I am approaching this correctly and would greatly appreciate any guidance on how to resolve this discrepancy.

holidays\countries\afghanistan.py

country = "AF"
default_language = "fa"
supported_languages = ("fa", "ps")

README.rst

   * - Afghanistan
     - AF
     -
     - **fa**, ps
     -

@KJhellico
Copy link
Collaborator

@Prateekshit73 , can you show the error in detail? To understand which test is failing.

@Prateekshit73
Copy link

Error:-
=============================== short test summary info =================================
FAILED tests/test_utils.py::TestListLocalizedEntities::test_localized_countries - AssertionError: Lists differ: ['fa'] != ['fa', 'ps']
======================== 1 failed, 3191 passed, 4 skipped in 26.52s =============================


tests\test_utils.py:157: in assertLocalizedEntities
self.assertEqual(
E AssertionError: Lists differ: ['fa'] != ['fa', 'ps']
E
E Second list contains 1 additional elements.
E First extra element 1:
E 'ps'
E
E - ['fa']
E + ['fa', 'ps'] : The supported languages for AF don't match its actual languages: set()

@KJhellico
Copy link
Collaborator

Do you have file holidays/locale/ps/LC_MESSAGES/AF.po?

@Prateekshit73
Copy link

There is no ps folder present. How can I generate it? I have gone through all the instructions in holidays/CONTRIBUTING.rst.

@KJhellico
Copy link
Collaborator

You need to create folders in this path and place .po file there:

Copy the generated template to all locale folders you're going to translate this country holiday names into (e.g., for Argentina: holidays/locale/en/LC_MESSAGES/AR.po - note the file extension difference here). Also copy the template to a default country language folder (e.g., for Argentina holidays/locale/es/LC_MESSAGES) and leave it as is. After copying the .po files open them with your favorite .po file editor and translate accordingly. Don't forget to fill in the translation file headers.

(quote from CONTRIBUTING.rst)

@Prateekshit73
Copy link

Sorry for overlooking that; I'm still a newbie trying to contribute. Do I need to copy the AF.po file to this location: holidays/locale/ps/LC_MESSAGES/AF.po?

@KJhellico
Copy link
Collaborator

KJhellico commented Dec 26, 2024

Yes, AF.po with Pashto translation should be there. You can make it from AF.pot or AF.po (with Farsi translation), as you prefer. Feel free to ask anything you don't quite understand.

@arkid15r
Copy link
Collaborator Author

Yes, as @KJhellico mentioned if you declare support for both fa and ps you actually need to do the work and implement it. holidays/locale/ps/LC_MESSAGES/AF.po file must contain ps translation. Please also consider making it ps_AF if needed.

@Prateekshit73
Copy link

Sorry for my silly mistakes, but I encountered a new error after implementing holidays/locale/ps_AF/LC_MESSAGES/AF.po. Here is the error:

ERROR: Coverage failure: total of 99 is less than fail-under=100
vacanza_holidays\.venv\Lib\site-packages\pytest_cov\plugin.py:355: CovFailUnderWarning: Coverage failure: total of 99 is less than fail-under=100
warnings.warn(CovFailUnderWarning(message), stacklevel=1)
[100%]

---------- coverage: platform win32, python 3.13.1-final-0 -----------
Name Stmts Miss Branch BrPart Cover
----------------------------------------------------------------------------------------------
holidays\__init__.py 8 0 0 0 100%
holidays\countries\__init__.py 156 0 0 0 100%
holidays\countries\afghanistan.py 49 1 26 7 89%
holidays\registry.py 52 0 14 0 100%
----------------------------------------------------------------------------------------------
TOTAL 11775 1 3400 7 99%
Coverage XML written to file coverage.xml

FAIL Required test coverage of 100% not reached. Total coverage: 99.95%
===================== 3192 passed, 4 skipped in 24.22s ================
make: *** [Makefile:69: test] Error 1

@arkid15r
Copy link
Collaborator Author

We've started enforcing 100% test coverage recently -- you need to add tests for your code and they must cover all of it.

It seems your code is ready for PR. Please create one and we'll take care of the issues together.

Thank you!

@Prateekshit73
Copy link

While pushing changes, it automatically runs make check, which causes the tests to fail for the same reason, preventing the push from succeeding.

@KJhellico
Copy link
Collaborator

KJhellico commented Dec 27, 2024

You can temporarily delete pre-push hook:

pre-commit uninstall --hook-type pre-push

Then, when coverage issue resolves, return it back:

pre-commit install --hook-type pre-push

@arkid15r
Copy link
Collaborator Author

While pushing changes, it automatically runs make check, which causes the tests to fail for the same reason, preventing the push from succeeding.

It sounds like you need add --no-verify flag.

@KJhellico KJhellico linked a pull request Dec 28, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants