refactor: views: Add year in message info view. #930
Conversation
jalajcodes
force-pushed
the
msg-info-popup
branch
from
e5c141d to
45fda74
Compare
neiljp
added
PR awaiting update
There was a problem hiding this comment.
This does seem to work OK, but there are some general aspects in general you might wanna know 👍
It is generally preferred to keep a refactor commit in the beginning. One way this helps is to avoid changing related code in multiple commits (referring to one of my comments below). If you think this is something we could add in the docs, do let us know :)
The show_year param might also not need a default value (coz we're validating it anyway).
| return local_time.strftime('%a %b %-d %Y %-H:%M:%S') | ||
| else: | ||
| return local_time.strftime('%a %b %d %H:%M') | ||
| format_codes = ( |
There was a problem hiding this comment.
It's a good practice to make intermediate variable names context dependent, in general. Something like "format_date" could work?
| @@ -1216,7 +1216,8 @@ def __init__(self, controller: Any, msg: Message, title: str, | |||
| self.message_links = message_links | |||
| self.time_mentions = time_mentions | |||
| date_and_time = controller.model.formatted_local_time( | |||
| msg['timestamp'], show_year=True) | |||
| msg['timestamp'], show_seconds=True, show_year=True | |||
There was a problem hiding this comment.
You might have observed, that you had to change the parameter you added, in both the commits, when you refactor in a later commit. See my review comment for reference :)
There was a problem hiding this comment.
Sorry, I didn't quite catch what you're trying to say. What do you mean by refactor in a later commit? I didn't have to make any changes in this file in the second commit.
There was a problem hiding this comment.
Ah, that is surely a confusion. See my comment here Apologies 😅
Can you please elaborate?
I don't think so, as removing the default value will make the |
Sure, For instance, I have a commit that adds a key-value pair in a dictionary. As a refactor, I want that dict to convert into an OrderedDict. So, ideally, what I wish to do is to convert that dict to OrderedDict prior to adding key-value pairs in it. To compare with your code, you've added the
Ah I see, I thought we're adding |
zulipterminal/model.py
Outdated
| local_time = datetime.datetime.fromtimestamp(timestamp) | ||
| if show_seconds: | ||
| return local_time.strftime('%a %b %d %H:%M:%S') | ||
| if show_year: |
There was a problem hiding this comment.
I was referring this addition. Notice that you're indirectly modifying this condition in the later commit (where you're declaring intermediate variable). However, since code-wise this is an insignificant change, I guess we shouldn't bother about discussing it more :)
|
Heads up @jalajcodes, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
@jalajcodes Thanks for working on this! 👍 I just squashed these commits together, made a little reformatting and merged as 74a78be 🎉 I squashed since the commits were very small, though refactoring to put the f-string conditional in-line might have been one option. There was a minor bug in the output with regard to spaces which I fixed - though we should probably add a test for this method to prevent introducing this kind of regression :) |
Added Year in the message info popup box.
Fixes #879