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

Add added option to _print_edited_summary #1366

Merged
merged 5 commits into from
Nov 20, 2021

Conversation

piero-vic
Copy link
Contributor

Fixes #1198. The summary used to show negative numbers when entries were added. There was also no option to show the added entries.

Checklist

  • I have read the contributing doc.
  • I have included a link to the relevant issue number.
  • I have checked to ensure there aren't other open pull requests
    for the same issue.
  • I have written new tests for these changes, as needed.

Copy link
Member

@micahellison micahellison left a comment

Choose a reason for hiding this comment

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

Hi there, thanks for this contribution! I'd love to see some tests with this, though I recognize that haven't written any tests for this in the past.

We have two methods for testing this that could be fine - the normal pytest unit tests in tests/unit, or the pytest-bdd tests in tests/bdd. You're welcome to take either approach.

And if you're not interested in adding that in, that's absolutely okay. Just please let us know so we can take that on.

@micahellison micahellison added the bug Something isn't working label Nov 6, 2021
@piero-vic
Copy link
Contributor Author

Hi. Thanks for the welcoming response. Unfortunately, I'm not familiar with testing in python. I would love to learn. Please feel free to add anything to the pull request. I will take a look at the changes and use them as an example for future contributions.

@micahellison micahellison dismissed their stale review November 13, 2021 23:02

Added tests

@micahellison
Copy link
Member

OK, I added in some BDD (Gherkin) tests, though I excluded DayOne since it has its own way of dealing with this issue, which I think is beyond the scope of the PR.

I also ran into some cache issues (hence the failing tests in GitHub actions) but resolved them in #1373.

Copy link
Member

@wren wren left a comment

Choose a reason for hiding this comment

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

🚀

@micahellison micahellison merged commit ba3fd22 into jrnl-org:develop Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong delete count when manually changing entries
3 participants