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 Armenian locale #1096

Merged
merged 10 commits into from
Jun 23, 2022
Merged

Add Armenian locale #1096

merged 10 commits into from
Jun 23, 2022

Conversation

ElahehAx
Copy link
Contributor

@ElahehAx ElahehAx commented Mar 2, 2022

Pull Request Checklist

Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:

  • 🧪 Added tests for changed code.
  • 🛠️ All tests pass when run locally (run tox or make test to find out!).
  • 🧹 All linting checks pass when run locally (run tox -e lint or make lint to find out!).
  • 📚 Updated documentation for changed code.
  • ⏩ Code is up-to-date with the master branch.

If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!

Description of Changes

Add Armenian locale

@codecov
Copy link

codecov bot commented Mar 5, 2022

Codecov Report

Merging #1096 (e0ae368) into master (7b5c1aa) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1096   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         2314      2325   +11     
  Branches       448       449    +1     
=========================================
+ Hits          2314      2325   +11     
Impacted Files Coverage Δ
arrow/constants.py 100.00% <ø> (ø)
arrow/locales.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b5c1aa...e0ae368. Read the comment docs.

@anishnya
Copy link
Member

anishnya commented Mar 6, 2022

Hey @ElahehAx, the PR looks good to me. I'm not too sure why CodeCov says there's reduced coverage. I'll have to investigate further. @jadchaar @krisfremen @systemcatch any thoughts here? I don't see anything that would cause a coverage decrease.

@anishnya anishnya requested a review from jadchaar March 11, 2022 18:22
@systemcatch
Copy link
Collaborator

Are we missing tests for the merdians? We dropped the coverage requirement below 100% recently but can't remember the exact figure.

@anishnya
Copy link
Member

Are we missing tests for the merdians? We dropped the coverage requirement below 100% recently but can't remember the exact figure.

Looking at the CodeCov coverage, it looks like it says there's a gap in the humanize coverage? To me this doesn't make sense. Will need to investigate further.

@ElahehAx
Copy link
Contributor Author

Hi. Thanks for your comments. Because of systemcatch's comment, I added some tests for meridians. I also deleted some duplicate tests.

@ElahehAx
Copy link
Contributor Author

Hi, I saw that these tests failed, however the local test on my machine was successful,would you suggest anything I can do to resolve this issue?

@anishnya
Copy link
Member

Hi, I saw that these tests failed, however the local test on my machine was successful,would you suggest anything I can do to resolve this issue?

So it looks like re-running the tests solved that issue. The coverage issue appears to be the fact CodeCov thinks coverage has been reduced on a specific branch of humanize. I'll investigate this later today, but I suspect it might be something to do with the way Dehumanize uses Humanize in testing that's causing this.

@cyriaka90 cyriaka90 mentioned this pull request Apr 20, 2022
5 tasks
@systemcatch
Copy link
Collaborator

Hey @ElahehAx do you want to fix the conflicts or shall we do it? Our CI issue is hopefully fixed now. 🤞

@ElahehAx
Copy link
Contributor Author

ElahehAx commented May 9, 2022

hey @systemcatch of course. Please feel free to do it

Copy link
Collaborator

@systemcatch systemcatch left a comment

Choose a reason for hiding this comment

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

Hey @ElahehAx i fixed the conflicts but made 2 small syntax errors. If you commit the suggestions that should fix everything.

arrow/locales.py Outdated Show resolved Hide resolved
arrow/locales.py Outdated Show resolved Hide resolved
@ElahehAx
Copy link
Contributor Author

Hey @systemcatch and @anishnya, I updated the PR (conflicts and syntax error solved), but now "Lint docs" (https://github.com/arrow-py/arrow/runs/6741399878?check_suite_focus=true) has an error. Do you know please why it's failing?"

@krisfremen
Copy link
Member

hey @ElahehAx it seems to be an issue with the docs generation.

It doesn't seem to be anything from your code though. Will take a look shortly!

@krisfremen
Copy link
Member

hey @ElahehAx, seems like the culprit was sphinx, got a PR ready to fix that, as soon as it's in, I'll rerun the checks and merge this in.

Copy link
Member

@krisfremen krisfremen left a comment

Choose a reason for hiding this comment

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

LGTM

@krisfremen krisfremen merged commit dc6428f into arrow-py:master Jun 23, 2022
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.

None yet

5 participants