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

Added warning message for todo.sh archive #399

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

Conversation

chrysle
Copy link
Contributor

@chrysle chrysle commented Dec 18, 2022

I have added a warning message which todo.sh archive will print out if it does not find done tasks in the todo file.
Before, it would even then print TODO: ./todo.txt archived.

Before submitting a pull request, please make sure the following is done:

  • Fork the repository and create your branch from master.
  • If you've added code that should be tested, add tests!
  • Ensure the test suite passes.
  • Format your code with ShellCheck.
  • Include a human-readable description of what the pull request is trying to accomplish.
  • Steps for the reviewer(s) on how they can manually QA the changes.
  • Have a fixes #XX reference to the issue that this pull request fixes.

Copy link
Member

@inkarkat inkarkat left a comment

Choose a reason for hiding this comment

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

I like the idea; it's nice to give precise messages about what has been done (or not); especially when doing so does not require extra work - the grep command already tells us whether there are done tasks. However:

After your change, archiving becomes non-functional if TODOTXT_VERBOSE=0; line 1145 just echos the done tasks if verbose is on; I think you need to use the following line's grep to check for done tasks. The tests (make test) are also failing - please add a test for your warning message to tests/t1900-archive.sh.

Copy link
Member

@inkarkat inkarkat 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 adding tests! (The test failures can be ignored; there's a problem with MacOS platforms and apparently now also with Ubuntu 18.04.) I think we're almost there now.

todo.sh Outdated Show resolved Hide resolved
@chrysle
Copy link
Contributor Author

chrysle commented Dec 21, 2022

Should I squash my commits?

@inkarkat
Copy link
Member

You can squash if you want to, but there's no need. The change is looking fine now, thanks a lot!

It'll probably still take some time (at least until January) until this gets merged, we currently have a bit of a backlog of PRs, but @karbassi has a New Year's resolution of devoting some time to this project, and we'll very likely also do a fresh release then ;-)

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.

2 participants