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

Allow the next break or microbreak to be scheduled from the CLI #1092

Merged
merged 4 commits into from
Feb 21, 2022
Merged

Allow the next break or microbreak to be scheduled from the CLI #1092

merged 4 commits into from
Feb 21, 2022

Conversation

lkrms
Copy link
Contributor

@lkrms lkrms commented Jan 24, 2022

Issue: closes #1091

Requirements

  • issue was opened to discuss proposed changes before starting implementation. It is important do discuss changes before implementing them (Why should we add it? How should it work? How should it look? Where will it be? ...).
  • during development, node version specified in package.json was used (ie using nvm).
  • package versions and package-lock.json were not changed (npm install --no-save).
  • app version number was not changed.
  • all new code has tests to ensure against regressions.
  • npm run lint reports no offenses.
  • npm run test is error-free.
  • README and CHANGELOG were updated accordingly.
  • after PR is approved, all commits in it are squashed

Description of the Change

In the Command class:

  • Add new --wait option to commands mini and long.
  • Add waitToMs method to utilise the existing interval parser (parseDuration) from main.js when processing CLI options.

BreaksPlanner:

  • Rename skipToMicrobreak to scheduleMicrobreak and modify it slightly, adding a new skipToMicrobreak function that simply calls scheduleMicrobreak(100)

  • Ditto for skipToBreak and scheduleBreak

  • Fix a few minor typos and inconsistencies in logged output.

Copy link
Owner

@hovancik hovancik left a comment

Choose a reason for hiding this comment

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

Thanks! I added some comments

app/breaksPlanner.js Show resolved Hide resolved
app/breaksPlanner.js Outdated Show resolved Hide resolved
app/main.js Outdated Show resolved Hide resolved
app/main.js Outdated Show resolved Hide resolved
app/utils/commands.js Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #1092 (cf9342a) into trunk (9614c13) will decrease coverage by 0.10%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1092      +/-   ##
==========================================
- Coverage   78.44%   78.33%   -0.11%     
==========================================
  Files          14       14              
  Lines         334      337       +3     
==========================================
+ Hits          262      264       +2     
- Misses         72       73       +1     
Impacted Files Coverage Δ
app/utils/commands.js 55.00% <40.00%> (+0.36%) ⬆️

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 9614c13...cf9342a. Read the comment docs.

@lkrms
Copy link
Contributor Author

lkrms commented Feb 2, 2022

I've made the requested changes in a new commit. To avoid losing context, I didn't rebase or squash, but am happy to if needed.

Copy link
Owner

@hovancik hovancik left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I've tried locally and all seems fine :) Could you please update the CHANGELOG and squash?

lkrms and others added 4 commits February 21, 2022 14:53
`Command`:
- Add new `--wait` option to commands `mini` and `long`
- Add `waitToMs` method to utilise the existing interval parser,
  `parseDuration`

`BreaksPlanner`:
- Split `skipToMicrobreak` to add `scheduleMicrobreak`
- Same for `skipToBreak`, `scheduleBreak`

- Fix typos and inconsistencies in logged output
- breaksPlanner.js: remove separate `scheduleMicrobreak` and
  `scheduleBreak` functions
- main.js:
  - Give `ms` variables more descriptive names
  - Remove default `delay` from `skipToMicrobreak` and `skipToBreak`
- commands.js: add an example
@hovancik hovancik merged commit e180a08 into hovancik:trunk Feb 21, 2022
@lkrms
Copy link
Contributor Author

lkrms commented Feb 21, 2022

Thanks for the changes! I've tried locally and all seems fine :) Could you please update the CHANGELOG and squash?

I'm sorry I didn't get to this sooner. FOSS contribution time has been difficult to find over the last couple of weeks, but I was happy to do it. Thank you for merging. 🙏🏻

@hovancik
Copy link
Owner

No worries :) I know the feeling, on the same boat here

@lkrms lkrms deleted the feature/schedule-next-break branch June 20, 2022 02:47
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.

[Feature request]: Allow the next break or microbreak to be scheduled from the CLI
2 participants