-
-
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 linewrap option 'auto' #1507
Conversation
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.
I have a small nitpick on the except clause, and I think this also needs some tests, either a unit test or a BDD test. Let us know if you'd like any help with that.
jrnl/Entry.py
Outdated
if columns == "auto": | ||
try: | ||
columns = os.get_terminal_size()[0] | ||
except: # noqa: E722 |
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.
Could you use except OSError
here? The docs for get_terminal_size
say that's the exception that fires when it can't get the terminal size for standard out, I'd like to avoid generic exceptions.
Thanks for your remarks! I can easily specify the except clause. |
We're working on the finishing touches of the 3.0 beta and are going to prioritize that now, but I'll get back to this PR to review in a week or so. Thanks for your work so far on it! |
That's no problem, I'm happy to contribute where I can. |
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.
One minor issue - this is broken with the "fancy" format. Could you take a look? It has its own code in jrnl/plugins/fancy_exporter.py. Thanks!
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.
Thanks for making this change. It's working well for me. I don't like seeing the same code in multiple places at once, but this is a more of a problem with jrnl's architecture than with your PR. We eventually want to refactor the Journal/Entry classes as mentioned in #1184 to separate presentation from data travel, and that's way outside of the scope of this PR.
You're welcome, glad I can contribute! I'm also not a fan of duplicate code myself. But to fix it would need to create a function in a shared file with another import as consequence. That wouldn't be beautiful too. When the refactoring takes place, it will resolve issues like this indeed. |
This resolves the feature request #1246 It adds the possibility to set 'auto' as linewrap option so jrnl automatically determines the width of the terminal/columns.
I chose to do the check while outputting information. The downside is that when using the debug flag it outputs a line for every entry. But it could also be done earlier when the config file is being read. Would that be better? If so, I'll modify the code to do so.
Fixes #1246
Checklist
for the same issue.