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

feat(1090/default_tz): Added possibility to override default UTC - #1… #1122

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

Conversation

morcef
Copy link

@morcef morcef commented Aug 17, 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

#1090 - Added possibility to override default UTC timezone. I don't believe this should be breaking anything, but I appretiate any feedback as this is my first PR on such a project.

Closes: #1090

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1122 (b087369) into master (f8f3068) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1122   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         2325      2336   +11     
  Branches       449       446    -3     
=========================================
+ Hits          2325      2336   +11     
Impacted Files Coverage Δ
arrow/api.py 100.00% <ø> (ø)
arrow/arrow.py 100.00% <100.00%> (ø)
arrow/constants.py 100.00% <100.00%> (ø)
arrow/factory.py 100.00% <100.00%> (ø)
arrow/util.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@morcef
Copy link
Author

morcef commented Aug 30, 2022

Thank you @anishnya for approving the workflow.
I must admit that I am new to the whole contribution part, so this is my first contribution in form of an actual PR.
If I understood the previous workflow, it failed on two linting errors in two files, and the coverage was reduced.
I have tried to fix theses in my new commits. Please let me know if there is anything else that needs improvement/fixing :)

@anishnya
Copy link
Member

the previous workflow, it failed on t

No worries. Thank you for contributing! Please feel free to ask us any questions and we'll be more than happy to help! :)

@morcef
Copy link
Author

morcef commented Sep 4, 2022

@anishnya , my apologies, I did not catch the isort and black failures.
I believe I should have sorted out all the errors here now 🤞

@morcef
Copy link
Author

morcef commented Sep 6, 2022

Oh my!
I did not notice that docstring was removed..
@anishnya , I am not fully sure what caused the segmentation fault in the macos tests, but hopefully this was just a glitch in the system?

@morcef
Copy link
Author

morcef commented Nov 4, 2022

Hi @anishnya,
There seems to have appeared a conflict with the documentation here. I guess the documentation has been updated since I pushed my code. Is there much going on with the code base these days? Has there been/is there any chance to take a look at this PR?
Thanks

@krisfremen
Copy link
Member

Hi @anishnya,
There seems to have appeared a conflict with the documentation here. I guess the documentation has been updated since I pushed my code. Is there much going on with the code base these days? Has there been/is there any chance to take a look at this PR?
Thanks

Hey @morcef, we did move things around in the documentation recently to improve readability.

I try to get stuff reviewed and merged in when free time pops up. I will take a look at this PR in the next few days.

@morcef
Copy link
Author

morcef commented Nov 9, 2022

Thank you @krisfremen :)
In regards to the documentation conflict, would you like me to look into that, or would it be better for you to just place my changes according to your recent improvement?

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.

5 participants