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

Inconsistency in period expressions interpretations #650

Open
ony opened this Issue Nov 21, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@ony
Collaborator

ony commented Nov 21, 2017

I've noticed weird inconsistency in treating period expressions:

  • every 1st day of month from 2017/1/5: starts from 2017/2/1 (valid)
  • monthly from 2017/1/5: starts from 2017/1/1 (outside of the range)

Though should be easy to fix.

@ony ony added the A BUG label Nov 21, 2017

@ony ony changed the title from Inconsistency in periods treatment to Inconsistency in period expressions interpretations Nov 21, 2017

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Nov 21, 2017

Collaborator

If we want to make monthly to be day-agnostic. Then we probably should have behaviour like:

monthly from 2017/1/5: gives dates 2017/1/5, 2017/2/1, 2017/3/1 ....

Collaborator

ony commented Nov 21, 2017

If we want to make monthly to be day-agnostic. Then we probably should have behaviour like:

monthly from 2017/1/5: gives dates 2017/1/5, 2017/2/1, 2017/3/1 ....

@ony ony removed the A BUG label Nov 21, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 21, 2017

ghost commented Nov 21, 2017

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Nov 21, 2017

Collaborator

I think i fixed this in runPeriodicTransactioms in #645

Collaborator

adept commented Nov 21, 2017

I think i fixed this in runPeriodicTransactioms in #645

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Nov 21, 2017

Collaborator

Posted too soon, before reading your comments there :(

Collaborator

adept commented Nov 21, 2017

Posted too soon, before reading your comments there :(

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Nov 21, 2017

Collaborator

@hledgerlist , you are right I'm refering to splitSpan used in multi-period reports. Ideally we should specify monthly from 2017/1.
I see that it makes rendering of column headers much better than having precise boundaries of first and last period. It just feels a bit unnatural for this function.
If it only made like this for report we probably should truncate days/month for rendering.

I would expect all next commands be equivalent:

  • hledger bal -p 'monthly from 2017/11/15'
  • hledger bal -p 'monthly' -b 2017/11/15
  • hledger bal -M -b 2017/11/15
  • hledger bal -M date:'from 2017/11/15'

And -b as well as date (part of query) to be honored precisely.
But it is too late for such decision, I guess. So we can close this ticket (that's why I dropped "bug" label).

Collaborator

ony commented Nov 21, 2017

@hledgerlist , you are right I'm refering to splitSpan used in multi-period reports. Ideally we should specify monthly from 2017/1.
I see that it makes rendering of column headers much better than having precise boundaries of first and last period. It just feels a bit unnatural for this function.
If it only made like this for report we probably should truncate days/month for rendering.

I would expect all next commands be equivalent:

  • hledger bal -p 'monthly from 2017/11/15'
  • hledger bal -p 'monthly' -b 2017/11/15
  • hledger bal -M -b 2017/11/15
  • hledger bal -M date:'from 2017/11/15'

And -b as well as date (part of query) to be honored precisely.
But it is too late for such decision, I guess. So we can close this ticket (that's why I dropped "bug" label).

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Nov 21, 2017

Collaborator

And I gues current behavior is definitely more useful in terms of "I want to see week report that includes that days" (hledger bal -p 'weekly in today').

Collaborator

ony commented Nov 21, 2017

And I gues current behavior is definitely more useful in terms of "I want to see week report that includes that days" (hledger bal -p 'weekly in today').

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Nov 26, 2017

Collaborator

I didn't found in documentation any mention about extending ranges to include complete periods. The only place is code documentation for splitSpan:

-- | Split a DateSpan into one or more consecutive whole spans of the specified length which enclose it.
-- If no interval is specified, the original span is returned.

It might be not clear from a frist glance, but I believe it states that it produce consecutive spans of specified length that together enclose provided DateSpan. This means that behaviour of every 1st day of month from 2017/1/5 should actually include date 2017/1/5 in one of the produced spans like monthly from 2017/1/5 do.

This behaviour becomes consistent with change 4049455 which is part of #654 .

Collaborator

ony commented Nov 26, 2017

I didn't found in documentation any mention about extending ranges to include complete periods. The only place is code documentation for splitSpan:

-- | Split a DateSpan into one or more consecutive whole spans of the specified length which enclose it.
-- If no interval is specified, the original span is returned.

It might be not clear from a frist glance, but I believe it states that it produce consecutive spans of specified length that together enclose provided DateSpan. This means that behaviour of every 1st day of month from 2017/1/5 should actually include date 2017/1/5 in one of the produced spans like monthly from 2017/1/5 do.

This behaviour becomes consistent with change 4049455 which is part of #654 .

@ony ony reopened this Nov 26, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Nov 27, 2017

Owner

Agreed, this should be mentioned in docs.

Re "monthly from 2017/1/5" - I thought after #654 maybe this would give an error (interval is monthly but does not start on a month boundary) but it didn't. I guess only the "every ..." form checks that.

If we did want to support intervals not starting on the natural boundary (and I don't know how big a task that is), I guess we'd expect "monthly from 2017/1/5" to generate 2017/1/5, 2017/2/5, 2017/3/5 etc. ?

That gets into what to do about "monthly from 2017/1/30" etc. and all of the date-adjusting options Carel mentioned. If we go further down this route we should probably consider closely following the semantics (at least) of ISO 8601 and avoid reinventing everything.

Owner

simonmichael commented Nov 27, 2017

Agreed, this should be mentioned in docs.

Re "monthly from 2017/1/5" - I thought after #654 maybe this would give an error (interval is monthly but does not start on a month boundary) but it didn't. I guess only the "every ..." form checks that.

If we did want to support intervals not starting on the natural boundary (and I don't know how big a task that is), I guess we'd expect "monthly from 2017/1/5" to generate 2017/1/5, 2017/2/5, 2017/3/5 etc. ?

That gets into what to do about "monthly from 2017/1/30" etc. and all of the date-adjusting options Carel mentioned. If we go further down this route we should probably consider closely following the semantics (at least) of ISO 8601 and avoid reinventing everything.

@adept

This comment has been minimized.

Show comment
Hide comment
@adept

adept Nov 27, 2017

Collaborator
Collaborator

adept commented Nov 27, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Nov 27, 2017

Owner

Related, @eamsden says currently "biweekly" must start on Sundays.

Period expressions are in need of new documentation and systematic review.

Owner

simonmichael commented Nov 27, 2017

Related, @eamsden says currently "biweekly" must start on Sundays.

Period expressions are in need of new documentation and systematic review.

@ehildenb

This comment has been minimized.

Show comment
Hide comment
@ehildenb

ehildenb Sep 3, 2018

Contributor

Second sticking to ISO8601 standard, existing date does, eg (interaction with shell):

[hledger]% date --date='2017/08/05 + 1 day'
Sun Aug  6 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 8 day'
Sun Aug 13 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 1 week'
Sat Aug 12 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 1 month'
Tue Sep  5 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 1 month + 1 day'
Wed Sep  6 00:00:00 CDT 2017
[hledger]% date --date='2017/01/30 + 1 month'        
Thu Mar  2 00:00:00 CST 2017
Contributor

ehildenb commented Sep 3, 2018

Second sticking to ISO8601 standard, existing date does, eg (interaction with shell):

[hledger]% date --date='2017/08/05 + 1 day'
Sun Aug  6 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 8 day'
Sun Aug 13 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 1 week'
Sat Aug 12 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 1 month'
Tue Sep  5 00:00:00 CDT 2017
[hledger]% date --date='2017/08/05 + 1 month + 1 day'
Wed Sep  6 00:00:00 CDT 2017
[hledger]% date --date='2017/01/30 + 1 month'        
Thu Mar  2 00:00:00 CST 2017
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Sep 3, 2018

Owner

@ehildenb, interesting, though are you sure date sticks to ISO8601 ? It seems to me it overlaps it, that's all. I found docs for the above at https://www.gnu.org/software/coreutils/manual/html_node/Relative-items-in-date-strings.html.

@adept's questions are a good start at a more comprehensive explanation of our semantics. My guess is we'll want to

  • make period expressions in queries and in periodic transactions more consistent/the same
  • not reject expressions just because they don't start on a tidy interval boundary
  • develop the docs, and the syntax if needed, to make all behaviour clearer, more consistent and more expressive
  • make sure we offer sufficient debug output for period expression troubleshooting
  • maybe experiment with an optional alternative syntax such as ISO8601, after the above is under control
Owner

simonmichael commented Sep 3, 2018

@ehildenb, interesting, though are you sure date sticks to ISO8601 ? It seems to me it overlaps it, that's all. I found docs for the above at https://www.gnu.org/software/coreutils/manual/html_node/Relative-items-in-date-strings.html.

@adept's questions are a good start at a more comprehensive explanation of our semantics. My guess is we'll want to

  • make period expressions in queries and in periodic transactions more consistent/the same
  • not reject expressions just because they don't start on a tidy interval boundary
  • develop the docs, and the syntax if needed, to make all behaviour clearer, more consistent and more expressive
  • make sure we offer sufficient debug output for period expression troubleshooting
  • maybe experiment with an optional alternative syntax such as ISO8601, after the above is under control
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment