Make start of week adjustable when using span #934

ALee008 · 2021-03-01T20:38:48Z

…ng span in combination with frame == "week"

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!). --> see comments below
🧹 All linting checks pass when run locally (run tox -e lint or make lint to find out!). --> see comment below
📚 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

Closes: #355
This is my first PR and I choose this issue because it was labeled as 'good first issue'. I added a new parameter weekday that can be used in combination with frame == "week" to choose a start day for arrow.span().

My tests weren't successful. There was an issue with Python3.6 and an error. I ran the tests again against the master code and encountered the same error, see screenshots:

Master Error:

Master Summary:

These where the only problems I encountered after implementing my changes.

Furthermore I had problems running the linting, I got the following message:

I hope we can resolve these issues. Thank you and kind regards.

…ng span in combination with frame == "week"

jadchaar · 2021-03-02T19:50:16Z

Hi @ALee008, thanks for the PR. I will try to review this in the next couple of weeks, I am pretty swamped at the moment. Hopefully @krisfremen or @systemcatch could review it in the meantime.

jadchaar · 2021-03-02T19:53:40Z

Also, the tests are currently failing because of an XFailed pytest test case that is decreasing coverage. I will submit a patch to get this resolved.

ALee008 · 2021-03-02T20:18:14Z

Hey @jadchaar ,
much appriciated. I am looking forward to your feedback.

codecov · 2021-03-02T20:52:31Z

Codecov Report

Merging #934 (d0b1a0d) into master (f18be7f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #934   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files          10       10           
  Lines        1944     1947    +3     
  Branches      312      313    +1     
=======================================
+ Hits         1939     1942    +3     
  Misses          4        4           
  Partials        1        1

Impacted Files	Coverage Δ
arrow/arrow.py	`99.00% <100.00%> (+<0.01%)`	⬆️

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 f18be7f...d0b1a0d. Read the comment docs.

systemcatch · 2021-03-02T21:13:09Z

Hey @ALee008 have you installed all the dependencies needed to run the tests? You need to install the following;

pip install tox
pip install pre-commit
pip install mypy
pip install -r requirements.txt

Also if you have tox already try tox -e lint.

ALee008 · 2021-03-02T22:07:46Z

Hey @ALee008 have you installed all the dependencies needed to run the tests? You need to install the following;
pip install tox
pip install pre-commit
pip install mypy
pip install -r requirements.txt
Also if you have tox already try tox -e lint.

Hey @systemcatch ,

I haven't installed these. Will do and re-test. Thank you.

ALee008 · 2021-03-02T22:31:19Z

Hey @systemcatch ,

all linting were successful but ´test_shift_kiritimati´ is still failing. How should I proceed now?

jadchaar · 2021-03-02T22:42:02Z

Try pulling the latest code @ALee008, it seems like the CI runs are only failing on linting now: https://github.com/arrow-py/arrow/pull/934/checks

systemcatch · 2021-03-02T22:45:17Z

@ALee008 what is the output of pip list?

ALee008 · 2021-03-02T23:26:36Z

@ALee008 what is the output of pip list?

My ´pip list´result (running in an virtual env).

Package Version

alabaster 0.7.12
appdirs 1.4.4
attrs 20.3.0
Babel 2.9.0
certifi 2020.12.5
cfgv 3.2.0
chardet 4.0.0
coverage 5.5
dateparser 1.0.0
distlib 0.3.1
docutils 0.16
filelock 3.0.12
identify 2.0.0
idna 2.10
imagesize 1.2.0
importlib-metadata 3.7.0
iniconfig 1.1.1
Jinja2 2.11.3
MarkupSafe 1.1.1
mypy 0.812
mypy-extensions 0.4.3
nodeenv 1.5.0
packaging 20.9
pip 21.0.1
pluggy 0.13.1
pre-commit 2.10.1
py 1.10.0
Pygments 2.8.0
pyparsing 2.4.7
pytest 6.2.2
pytest-cov 2.11.1
pytest-mock 3.5.1
python-dateutil 2.8.1
pytz 2019.3
PyYAML 5.4.1
regex 2020.11.13
requests 2.25.1
setuptools 54.0.0
simplejson 3.17.2
six 1.15.0
snowballstemmer 2.1.0
Sphinx 3.5.1
sphinx-autodoc-typehints 1.11.1
sphinxcontrib-applehelp 1.0.2
sphinxcontrib-devhelp 1.0.2
sphinxcontrib-htmlhelp 1.0.3
sphinxcontrib-jsmath 1.0.1
sphinxcontrib-qthelp 1.0.3
sphinxcontrib-serializinghtml 1.1.4
toml 0.10.2
tox 3.22.0
typed-ast 1.4.2
typing-extensions 3.7.4.3
tzlocal 2.1
urllib3 1.26.3
virtualenv 20.4.2
zipp 3.4.0

ALee008 · 2021-03-02T23:27:01Z

Try pulling the latest code @ALee008, it seems like the CI runs are only failing on linting now: https://github.com/arrow-py/arrow/pull/934/checks

Will do and retry.

arrow/arrow.py

systemcatch · 2021-03-04T19:16:24Z

arrow/arrow.py

+            >>> arrow.utcnow().span('week', weekday=6)
+            (<Arrow [2021-02-20T00:00:00+00:00]>, <Arrow [2021-02-26T23:59:59.999999+00:00]>)


Maybe compare this with a normal span("week") to highlight the difference.

Hey,
extended doc string in latest commit.

systemcatch · 2021-03-04T19:18:20Z

tests/test_arrow.py

@@ -1754,6 +1754,11 @@ def test_span_week(self):
        assert floor == datetime(2013, 2, 11, tzinfo=tz.tzutc())
        assert ceil == datetime(2013, 2, 17, 23, 59, 59, 999999, tzinfo=tz.tzutc())

+        floor, ceil = self.arrow.span("week", weekday=6)


We should also test what happens if a bad value is provided (i.e. week_start=55)

Co-authored-by: Chris <30196510+systemcatch@users.noreply.github.com>

systemcatch · 2021-03-06T09:57:02Z

arrow/arrow.py

+            >>> arrow.utcnow().span('week', week_start=1)
+            (<Arrow [2021-02-22T00:00:00+00:00]>, <Arrow [2021-02-28T23:59:59.999999+00:00]>)


No need for these line as it uses the default value above.

tests/test_arrow.py

ALee008 · 2021-03-21T23:05:54Z

Hey,
lint docs test failed. But I can't interpret the error message:

ERROR: InvocationError for command /usr/bin/make html 'SPHINXOPTS=-W --keep-going' (exited with code 2)
___________________________________ summary ____________________________________
ERROR:   docs: commands failed
Error: Process completed with exit code 1.

It didn't seem to fix the problem. Any suggestions?

systemcatch · 2021-03-22T11:20:44Z

Weird I don't understand why this is still failing, @jadchaar any ideas?

jadchaar · 2021-03-22T19:22:44Z

Yeah I fixed it--it was a trailing whitespace.

jadchaar · 2021-03-22T19:25:30Z

Hmm it seems like Github is rejecting my commit to this branch. I got an error about not having permission, so it seems to not be associated with a branch:

@ALee008 mind just removing that trailing whitespace on the line I changed in 4c1306a and pushing it?

ALee008 · 2021-03-25T20:36:39Z

Hmm it seems like Github is rejecting my commit to this branch. I got an error about not having permission, so it seems to not be associated with a branch:

@ALee008 mind just removing that trailing whitespace on the line I changed in 4c1306a and pushing it?

Will do an push.
Edit: I checked in an update 05512a6 but because of a conflict I merged the code which seems to have resulted in 9d84449 for which a check failed.

…to BR_choose_weekday_using_span

systemcatch · 2021-03-31T09:24:18Z

@jadchaar can you take a look at this again? I'm afk until Saturday night but will try to fix this then otherwise.

jadchaar · 2021-03-31T21:18:53Z

@jadchaar can you take a look at this again? I'm afk until Saturday night but will try to fix this then otherwise.

Yup fixed, it was a docs indent issue.

ALee008 · 2021-04-05T23:09:12Z

@jadchaar can you take a look at this again? I'm afk until Saturday night but will try to fix this then otherwise.

Yup fixed, it was a docs indent issue.

Hey @jadchaar,
what are the next steps? Do I need to do something now?

jadchaar · 2021-04-05T23:21:48Z

@jadchaar can you take a look at this again? I'm afk until Saturday night but will try to fix this then otherwise.

Yup fixed, it was a docs indent issue.

Hey @jadchaar,
what are the next steps? Do I need to do something now?

Hi @ALee008, apologies. I am still trying to understand the logic of finding the delta: delta = 7 if week_start > self.isoweekday() else 0. Chris brought up a good point with me: if week_start is 4 and isoweekday is 2, don't we need to go back 9 days? Also, it would be nice to expand the tests to cover more cases with more week_starts and more edge cases.

ALee008 · 2021-04-07T17:57:35Z

@jadchaar can you take a look at this again? I'm afk until Saturday night but will try to fix this then otherwise.

Yup fixed, it was a docs indent issue.

Hey @jadchaar,
what are the next steps? Do I need to do something now?

Hi @ALee008, apologies. I am still trying to understand the logic of finding the delta: delta = 7 if week_start > self.isoweekday() else 0. Chris brought up a good point with me: if week_start is 4 and isoweekday is 2, don't we need to go back 9 days? Also, it would be nice to expand the tests to cover more cases with more week_starts and more edge cases.

Hey @jadchaar,

so the way I interpret the requirement is as follows:

Be self an arrow object:

case 1: self.isoweekday() >= week_start --> this is the easy case because self.isoweekday() is already in that week.
case 2: self.isoweekday() < week_start --> the case in question: What should happen if self.isoweekday() is smaller than provided week_start? My interpretation: shift week_start one week back so that self is in this week.

Let's take Chris's example, week_start = 4(a Thursday) and self.isoweekday = 2(e.g. Tuesday 2021-04-06).

Because of case 2 the week would be spanned from Thursday 2021-04-01 to Thursday 2021-04-08.

Hope this makes my approach clearer.

jadchaar · 2021-04-07T22:17:48Z

Mind adding a few more test cases that cover both the cases you mention @ALee008? Then we can work to finally get this merged :).

ALee008 · 2021-04-08T21:06:19Z

Mind adding a few more test cases that cover both the cases you mention @ALee008? Then we can work to finally get this merged :).

Hey @jadchaar ,
I added more test cases. The test method might be a bit too verbose now, but it helped me keep track of what's going on :).

…to BR_choose_weekday_using_span

systemcatch

Thanks for your perseverance @ALee008, we're good to merge this in now.

jadchaar

LGTM

ALee008 · 2021-04-18T15:07:45Z

Thanks for your perseverance @ALee008, we're good to merge this in now.

I also have to thank you. You guys are doing a great job!

jadchaar · 2021-04-18T17:28:45Z

Thanks for your perseverance @ALee008, we're good to merge this in now.

I also have to thank you. You guys are doing a great job!

Thanks @ALee008 we appreciate it! We are trying to keep Arrow awesome :), but we cannot forget how amazing the community has been in helping us with that mission!

ALee008 added 3 commits March 1, 2021 01:14

added parameter weekday and thus ability to choose start day when usi…

e0266e1

…ng span in combination with frame == "week"

changed default value for new parameter weekday.

64f27aa

added test for new parameter weekday when using span and frame == "week"

dfd5a14

jadchaar requested review from krisfremen, systemcatch and jadchaar and removed request for krisfremen March 2, 2021 19:50

Merge branch 'master' into BR_choose_weekday_using_span

fc1a198

systemcatch requested changes Mar 3, 2021

View reviewed changes

arrow/arrow.py Outdated Show resolved Hide resolved

systemcatch changed the title ~~Br choose weekday using span~~ Make start of week adjustable when using span Mar 3, 2021

added comma after new parameter weekday.

647a67a

systemcatch requested changes Mar 4, 2021

View reviewed changes

ALee008 and others added 3 commits March 5, 2021 22:34

Update arrow/arrow.py

bd7cc7f

Co-authored-by: Chris <30196510+systemcatch@users.noreply.github.com>

changed parameter name from weekday to week_start.

964d804

extended doc string to highlight new parameter week_start.

59e467a

ALee008 requested a review from systemcatch March 5, 2021 22:57

systemcatch requested changes Mar 6, 2021

View reviewed changes

raise ValueError if 1 <= week_start <= 7 is not met

d2c4487

Remove trailing whitespace

4c1306a

ALee008 added 2 commits March 25, 2021 23:08

added new line at end of doc string.

05512a6

Merge remote-tracking branch 'origin/BR_choose_weekday_using_span' in…

9d84449

…to BR_choose_weekday_using_span

Fix doc issue

eba04fa

Merge branch 'master' into BR_choose_weekday_using_span

cbe796d

extended test_span_week by further test cases.

c8ee18a

Merge remote-tracking branch 'origin/BR_choose_weekday_using_span' in…

0f66a05

…to BR_choose_weekday_using_span

jadchaar requested review from systemcatch, jadchaar and krisfremen and removed request for krisfremen April 8, 2021 23:06

Merge branch 'master' into BR_choose_weekday_using_span

581cd1d

systemcatch approved these changes Apr 13, 2021

View reviewed changes

Merge branch 'master' into BR_choose_weekday_using_span

d0b1a0d

jadchaar approved these changes Apr 15, 2021

View reviewed changes

jadchaar merged commit 60bc196 into arrow-py:master Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make start of week adjustable when using span #934

Make start of week adjustable when using span #934

ALee008 commented Mar 1, 2021 •

edited by jadchaar

jadchaar commented Mar 2, 2021

jadchaar commented Mar 2, 2021

ALee008 commented Mar 2, 2021

codecov bot commented Mar 2, 2021 •

edited

systemcatch commented Mar 2, 2021

ALee008 commented Mar 2, 2021

ALee008 commented Mar 2, 2021

jadchaar commented Mar 2, 2021

systemcatch commented Mar 2, 2021

ALee008 commented Mar 2, 2021

ALee008 commented Mar 2, 2021

systemcatch Mar 4, 2021

ALee008 Mar 5, 2021

systemcatch Mar 4, 2021

systemcatch Mar 6, 2021

ALee008 commented Mar 21, 2021

systemcatch commented Mar 22, 2021

jadchaar commented Mar 22, 2021

jadchaar commented Mar 22, 2021

ALee008 commented Mar 25, 2021 •

edited

systemcatch commented Mar 31, 2021

jadchaar commented Mar 31, 2021

ALee008 commented Apr 5, 2021

jadchaar commented Apr 5, 2021

ALee008 commented Apr 7, 2021

jadchaar commented Apr 7, 2021

ALee008 commented Apr 8, 2021

systemcatch left a comment

jadchaar left a comment

ALee008 commented Apr 18, 2021

jadchaar commented Apr 18, 2021

		>>> arrow.utcnow().span('week', weekday=6)
		(<Arrow [2021-02-20T00:00:00+00:00]>, <Arrow [2021-02-26T23:59:59.999999+00:00]>)

		>>> arrow.utcnow().span('week', week_start=1)
		(<Arrow [2021-02-22T00:00:00+00:00]>, <Arrow [2021-02-28T23:59:59.999999+00:00]>)

Make start of week adjustable when using span #934

Make start of week adjustable when using span #934

Conversation

ALee008 commented Mar 1, 2021 • edited by jadchaar

Pull Request Checklist

Description of Changes

jadchaar commented Mar 2, 2021

jadchaar commented Mar 2, 2021

ALee008 commented Mar 2, 2021

codecov bot commented Mar 2, 2021 • edited

Codecov Report

systemcatch commented Mar 2, 2021

ALee008 commented Mar 2, 2021

ALee008 commented Mar 2, 2021

jadchaar commented Mar 2, 2021

systemcatch commented Mar 2, 2021

ALee008 commented Mar 2, 2021

ALee008 commented Mar 2, 2021

systemcatch Mar 4, 2021

Choose a reason for hiding this comment

ALee008 Mar 5, 2021

Choose a reason for hiding this comment

systemcatch Mar 4, 2021

Choose a reason for hiding this comment

systemcatch Mar 6, 2021

Choose a reason for hiding this comment

ALee008 commented Mar 21, 2021

systemcatch commented Mar 22, 2021

jadchaar commented Mar 22, 2021

jadchaar commented Mar 22, 2021

ALee008 commented Mar 25, 2021 • edited

systemcatch commented Mar 31, 2021

jadchaar commented Mar 31, 2021

ALee008 commented Apr 5, 2021

jadchaar commented Apr 5, 2021

ALee008 commented Apr 7, 2021

jadchaar commented Apr 7, 2021

ALee008 commented Apr 8, 2021

systemcatch left a comment

Choose a reason for hiding this comment

jadchaar left a comment

Choose a reason for hiding this comment

ALee008 commented Apr 18, 2021

jadchaar commented Apr 18, 2021

ALee008 commented Mar 1, 2021 •

edited by jadchaar

codecov bot commented Mar 2, 2021 •

edited

ALee008 commented Mar 25, 2021 •

edited