eza-community
feat(time_display): time should use timezone settings
When a file timestamp is displayed, it should use the timezone info from the time of the timestamp, not from when it is displayed. This occurs when a file timestamp is updated under daylight saving time, and displayed at a time when not under daylight saving time.
The bug is quite subtle, as it depends on the time of construction of the timestamp, not on the moment it is read.
This fixes issue #653 .
The pipeline is still not happy. I think another cargo fmt --all -- is in order. ;-)
The pipeline is still not happy. I think another
cargo fmt --all --is in order. ;-)
Yes, go a bit annoyed because only FreeBSD tests failed (different rustc settings?), and after some to-and-from work, I've forgotten to run cargo fmt. Sigh. Hope I've got everything this time.
Yea, I hear ya. I've run into BSD related issues myself. And don't get me started on illumos pipelines. No project should ever use those. Argh!
But, all tests pass! Yeah!
@cafkafk it's ready for review and/or merge.
I cannot speak for @erwinvaneijk but in my projects I don't care about the commit messages in a PR. What is important is that you don't do a merge commit, but rather a squash commit - and then use a convential commit message.
This means that the person who merges the PR can pick whatever commit message they want and there will be only one commit.
P.S.: Previously I asked people to rebase, squash, force push in PRs, but I no longer do that. It's easier to follow the development of the PR if this is not done. Especially when there are a lot of comments and change requests.
Glad you like the PR @gierens, thank you for the review.
However, the changes you request to the commit messages cannot be done (or my git skills are seriously lacking in this department), therefore this PR will keep the status 'changes requested' which I cannot fulfil. Or did I miss requested changes to the code itself?
That goes for me too. The merge to the main should be well-formatted, and it's a choice whether or not to squash or just merge. Trying to create a squash merge in this PR that then can be merged is a pain, IMNSHO.
@erwinvaneijk I can squash and force-push if the devs insist on it. but I need access/permissions to your repo/branch.
@cafkafk can you please have a look? the 2 current reviews didn't make much sense.
The first review would have let eza fail silently and as a result shown wrong dates/times without notifying the user.
The second review was about commit messages in this PR. Don't get me wrong, but why does gh have a Squash and merge button?
I am also ok with helping the author to squash the commits.
@erwinvaneijk I can squash and force-push if the devs insist on it. but I need access/permissions to your repo/branch.
I've added you to my repos, thanx for the effort you want to make.
Done. I've used feat because eza technically never claimed it would respect the timezone info.
@tessus sorry for the late reply, been a bit busy lately, otherwise I would've helped out with rebasing the commits.
the 2 current reviews didn't make much sense.
I try to elaborate a bit more, see below.
The second review was about commit messages in this PR. Don't get me wrong, but why does gh have a
Squash and mergebutton? I am also ok with helping the author to squash the commits.
As the CONTRIBUTING.md states, we like small and conventional commits that make a minimal self-contained change to the code. So for example, when adding a trait in one file and implementing it for some struct in another two commits would be a good choice. There is of course always room for interpretation. But doing a Squash-Merge on a PR that alters several things at once goes directly against that concept. If you do that in your repos that is fine, we do it differently though, which is also why we only have the Rebase option configured on the Merge button.
@gierens ah, ok. with CONTRIBUTING.md it makes more sense. in this case, this PR could have been squashed into 2 commits. The code change itself and the clippy fixes.
Anyway, I now have already squashed all commits into one. I don't have a backup.
P.S.: I didn't do a rebase. A rebase is not possible:
$ git rebase main
Current branch issue-653-2 is up to date.
@gierens ah, ok. with
CONTRIBUTING.mdit makes more sense.
Sorry for not explaining it very well before.
Anyway, I now have already squashed all commits into one. I don't have a backup.
No problem, I can quickly do that tomorrow but now I have to :sleeping:
A rebase is not possible:
$ git rebase main
Current branch issue-653-2 is up to date.
A rebase would only work, if you have commits in main after now 2025-01-17 23:35:01 +0000
P.S.: I didn't do a rebase. A rebase is not possible:
$ git rebase main Current branch issue-653-2 is up to date.
Sorry, I didn't mean rebase like that ... I meant picking it apart into multiple commits again, I usually use the term rebase as reference to interactive rebase git rebase -i ... which can be used for more intense history rewriting. So I use rebase as umbrella term for more intense history rewriting sometimes :D ... Not the clearest way to put it maybe, but it's getting late :D
But don't worry, I can quickly do that tomorrow.
There's no rebase -i possible anymore either. They are squashed. Or are you talking about using the reflog to do a hard reset?
To be honest those 2 additional commits make no sense.
A code change should not be taken apart, just to create commits. One commit should be about one code change (fix/feat/...). e.g.: this PR was about fixing the TZ behavior. But since there was no bug because there was no TZ code, it's actually a feature. The tests and adding crates to Cargo.toml are part of that change.
Additionally the code change wouldn't work without adding the crates as dependencies. Tests are usually also put in the same commit.
IMO this is not the idea behind conventional commits. Yes, you could have a second commit for the tests, but only if they were not in the first commit, because the author didn't write tests. But creating a separate commit just to use a build(deps) message is over the top.
I've been 30 years in IT and I have never seen a project that has such a weird commit strategy.
P.S.: I understand that one wants 2 commits, one for the change and another for the clippy stuff, but I would still merge it, if it were only 1 commit. So 2 commits are perfectlty valid. I would even be ok with 3 commits (tests in a separate commit), even though that's debatable. But these 4 commits are really, really weird.
To be honest those 2 additional commits make no sense.
A code change should not be taken apart, just to create commits. One commit should be about one code change (fix/feat/...). e.g.: this PR was about fixing the TZ behavior. But since there was no bug because there was no TZ code, it's actually a feature. The tests and adding crates to Cargo.toml are part of that change.
Everyone: thank you for the help getting this bug fixed. It doesn’t display the right time (under conditions), which exa did do properly. I’ve not spent time figuring out where the bug was introduced, but there you go.
If someone with the appropriate rights can press the merge button, that would be excellent!