-
-
Notifications
You must be signed in to change notification settings - Fork 524
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 calendar heatmap display format #1759
Conversation
I'm not sure how to resolve this last lint error:
It looks structurally identical to https://github.com/alichtman/jrnl/blob/bd921f2e0518f0051f7887d3ed42123e750fe26a/jrnl/journals/__init__.py#L1-L5 |
Thanks for the PR! I'm playing around with it and it looks nice. I like that it doesn't introduce any new dependencies, also.
It's in pyproject.toml:161. It should pass if you change that line to this (which I think is a good change):
The overall construction of this PR looks nice. I think the only things left to add are some BDD tests like those in formats.feature, and some unit tests in the new utility functions. |
I took a look at setting up some tests for this. The scaffolding for the test went into that last commit, but the test output is all misformatted, and I'm not sure why. At first glance, it seems like the output is being wrapped because it hits a character boundary. Except not all output is wrapped. See this example specifically. Why are the July calendar dates wrapped, but not the bar at the top of the calendar? And why aren't the numbers that should be below
...
The following works on my machine without any weird formatting. I don't know why the test is rendering it differently:
|
That's very strange. I'm not sure what's going on. It looks like pytest is treating the output fine in the GitHub Actions tests. On my Windows 10 machine, pytest is wrapping the output at 80 characters (well, 82 actually, but there it adds a 2-space indent before the output). Maybe there's something in your environment that it's reading through pytest? You might try it in the isolated tox environment:
When I do that, it wraps to the full width of my terminal (then breaks because pytest adds 2 more spaces to indent the output, but at least that's explicable). As for the root cause, I wonder if it has something to do with Rich's method for getting the terminal size. If you're not running Windows, it investigates STDIN, STDERR, and STDOUT for terminal size (console.py:1016), and uses the first terminal size it can get without an error. Maybe there's something going on there? |
I'm not sure when I'll have time to look into this. |
Following up on our Signal conversation: Since this is a non-critical component of the codebase, and none of us are going to invest the time in getting these tests working, I'm proposing shipping this diff without the tests. |
All tests pass.
|
@alichtman Hey, we're just getting everything caught up after being away for a while (updated dependencies today). We'll get this PR reviewed soon. Thanks for your patience! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for your patience!
I think this format is a lot more useful than the
termgraph
solution discussed in #743.Calendar Heatmap Exporter:
termgraph --calendar
:Checklist
for the same issue.