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.

Sign up for GitHub

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

Jump to bottom

feat: add getSiblingFixedPeriods #15

Merged
merged 1 commit into from
Mar 2, 2023
Merged

feat: add getSiblingFixedPeriods #15

merged 1 commit into from
Mar 2, 2023

Conversation

Mohammer5
Copy link
Contributor

Blocked by #13

@Mohammer5 Mohammer5 mentioned this pull request Feb 15, 2023
Merged
@Mohammer5 Mohammer5 force-pushed the get-following-and-previous-fixed-periods branch from 4f2055b to 39bf7b2 Compare February 15, 2023 11:41
@Mohammer5 Mohammer5 force-pushed the parse-fixed-period-id branch from 3253bd1 to 9f7beca Compare February 28, 2023 10:16
@Mohammer5 Mohammer5 force-pushed the get-following-and-previous-fixed-periods branch from 39bf7b2 to 795e64f Compare March 1, 2023 02:20
@Mohammer5 Mohammer5 force-pushed the parse-fixed-period-id branch 4 times, most recently from b6bdbea to 3e89eff Compare March 1, 2023 11:18
kabaros
kabaros reviewed Mar 1, 2023
View reviewed changes
Copy link
Collaborator

@kabaros kabaros left a comment

Choose a reason for hiding this comment

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

I have two general comments about approach / code:

  • code-wise, similar to previous PR, I think you can get rid of date comparisons and compare the date strings

  • regarding the approach, these two set of methods (previous vs following) seem so similar, almost identical in the case of yearly and daily, that it just feels like it's a lot of repetition there. There might be a case for separating them into two sets if they were conceptually different, but they also seem very similar conceptually for me - so any changes in logic for "previous" are likely to apply "following". Am I right in thinking they're conceptually very similar?

I don't think we always need to follow DRY for the sake of it but here specifically, it feels like we should.

  • Can you also clarify for me where these are used in the data-entry app (or point me to have a look) .. I think maybe that helps me understand some of the decisions here.

  • Regarding all the edge cases around start and end of years, would it have been possible to just populate the previous and subsequent year, along the current year ... and just lookup those when there is no match .. that feels cheaper (which is not very important) but simpler (which is important) than the workarounds we have now.

@Mohammer5 Mohammer5 force-pushed the get-following-and-previous-fixed-periods branch 2 times, most recently from 11dfbf2 to 7128937 Compare March 2, 2023 02:14
@Mohammer5 Mohammer5 changed the base branch from parse-fixed-period-id to alpha March 2, 2023 02:17
@Mohammer5 Mohammer5 force-pushed the get-following-and-previous-fixed-periods branch from 7128937 to a891c9f Compare March 2, 2023 05:16
@Mohammer5
Copy link
Contributor Author

code-wise, similar to previous PR, I think you can get rid of date comparisons and compare the date strings

✔️ Using string comparison now

I don't think we always need to follow DRY for the sake of it but here specifically, it feels like we should.

✔️ Merged into one function called getSiblingFixedPeriods

Can you also clarify for me where these are used in the data-entry app (or point me to have a look) .. I think maybe that helps me understand some of the decisions here.

  • Here is an example of getFixedPeriodByDate & getSiblingFixedPeriods (in that branch still getFollowingFixedPeriods)
  • Here is an example of getSiblingFixedPeriods (in that branch still getPreviousFixedPeriods)
  • Here is an example of createFixedPeriodFromPeriodId
  • Here is an example of endsBefore (in that branch still excludeDay)

Regarding all the edge cases around start and end of years, would it have been possible to just populate the previous and subsequent year, along the current year ... and just lookup those when there is no match .. that feels cheaper (which is not very important) but simpler (which is important) than the workarounds we have now.

Can you point me to what you're referring to?

@Mohammer5 Mohammer5 force-pushed the get-following-and-previous-fixed-periods branch 2 times, most recently from 98a4059 to 72be9e1 Compare March 2, 2023 09:33
@kabaros kabaros changed the title feat: add getFollowingFixedPeriods and getPreviousFixedPeriods feat: add getSiblingFixedPeriods Mar 2, 2023
@Mohammer5 Mohammer5 force-pushed the get-following-and-previous-fixed-periods branch from 72be9e1 to 23cd625 Compare March 2, 2023 09:45
@Mohammer5 Mohammer5 merged commit 59cb406 into alpha Mar 2, 2023
dhis2-bot added a commit that referenced this pull request Mar 2, 2023
@dhis2-bot
chore(release): cut 1.0.0-alpha.21 [skip ci]
b66dd81 
# [1.0.0-alpha.21](v1.0.0-alpha.20...v1.0.0-alpha.21) (2023-03-02)

### Features

* add getSiblingFixedPeriods ([#15](#15)) ([59cb406](59cb406))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0-alpha.21 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kabaros kabaros deleted the get-following-and-previous-fixed-periods branch March 9, 2023 12:54
dhis2-bot added a commit that referenced this pull request Jan 10, 2024
@dhis2-bot
chore(release): cut 1.1.0 [skip ci]
a583b90 
# [1.1.0](v1.0.2...v1.1.0) (2024-01-10)

### Bug Fixes

* **generate daily fixed periods:** use correct comparison for "endsBefore" ([#21](#21)) ([3cdb2be](3cdb2be))

### Features

* add createFixedPeriodFromPeriodId method ([#13](#13)) ([ef1132e](ef1132e))
* add getFixedPeriodByDate ([#14](#14)) ([9de82e0](9de82e0))
* add getSiblingFixedPeriods ([#15](#15)) ([59cb406](59cb406))
* **generate fixed periods:** add "endsBefore" option ([#20](#20)) ([6667c97](6667c97))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HendrikThePendric HendrikThePendric HendrikThePendric approved these changes

@kabaros kabaros kabaros approved these changes

Assignees
No one assigned
Labels
released on @alpha released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mohammer5 @dhis2-bot @HendrikThePendric @kabaros