search

projecthamster / hamster

GNOME time tracker
http://projecthamster.org
GNU General Public License v3.0
1.08k stars 249 forks source link

#671: convert str to Fact before check #672

Closed aquaherd closed 3 years ago

aquaherd commented 3 years ago

Please accept this PR into the next hamster release. It enables the xfce4-panel-plugin to do in-place edits without adding a json library dependency.

Original author is @ederag .

ederag commented 3 years ago

No, I do not have time to explain in details, but it's exactly where it should be, IMO.

matthijskooijman commented 3 years ago

No, I do not have time to explain in details, but it's exactly where it should be, IMO.

Hm, I'm not sure I understand why, but let's just leave it at this fix and leave any refactoring for later, then.

I've updated the commit message to be a bit more verbose and link to the isssue (@aquaherd this means I did a force push to your branch, so pull carefully).

As far as I'm concerned this is ready to merge once another approval is added (or, since it is a rather simple change, if nobody objects for some time, I guess).

ederag commented 3 years ago

I've updated the commit message to be a bit more verbose and link to the isssue

You play the boss, part of which I understand (you are responsible for the commits), but in doing so you lost my commit date, used a commit message that do not follow standard git conventions (both are important to me, and that's what future devs will see) and moreover, lost track that it was @aquaherd who made the work of rebasing my commit onto master. Your call, but to me, this is bad practice.

matthijskooijman commented 3 years ago

I agree that preserving sufficient history and crediting is important.

but in doing so you lost my commit date,

But the original authorship date is still used? It is shown by default in git show and if you show more details, you can see both the original authordate and the latest commitdate. There is no record of any intermediate commitdates, but I don't think git supports tracking those (your commit date was already lost in the rebase done by @aquaherd).

$ git show 864fa0fc4996d3eea3f10719e87d4068dcf2ae75 | grep Date
Date:   Thu Mar 11 21:10:01 2021 +0100
$ git show --pretty=fuller 864fa0fc4996d3eea3f10719e87d4068dcf2ae75 | grep Date
AuthorDate: Thu Mar 11 21:10:01 2021 +0100
CommitDate: Sat Mar 13 09:52:40 2021 +0100

used a commit message that do not follow standard git conventions

Which convention in particular? Standard git convention, AFAIK, is a single summary line, empty line and a detailed description On top of that I'm using a different convention than you have done in the past, in order to improve readability and verbosity.

and moreover, lost track that it was @aquaherd who made the work of rebasing my commit onto master.

Good point. This is normally covered by the Author attribute, but since @aquaherd is not the original author, their credit got lost. I've added a note to the commit message to fix that.

ederag commented 3 years ago

It feels longer to read and answer you than to fix things.

image

That's what devs see on github ("authored and committed 7 minutes ago"), and @aquaherd is visible only at the bottom of a wordy text.

matthijskooijman commented 3 years ago

That's what devs see on github ("authored and committed 7 minutes ago"),

As for the timestamp: This is a choice made by github (to show the commit date more prominently than the author date, unlike git, which prefers the AuthorDate), which is maybe unfortunate, but IMHO also not a big deal. It does not seem a good idea to me to provide an incorrect CommitDate for this commit just to get Github to display this nicer.

As for the crediting: If git would be able to list multiple committers, I'd be happy to do so, but the current approach is that git tracks the author and the most recent committer, so that's what's happening here. Again, I do not think it would be good to change the committer here, which would make it less clear who has finalized this commit.

ederag commented 3 years ago

would make it less clear who has finalized this commit.

In my opinion, you did little (but delaying) in comparison to @aquaherd.

ederag commented 3 years ago

@aquaherd You merged a commit I explicitly opposed to, which I find disrespectful, for your information. (it's not just about you, there's also the commit message itself)

aquaherd commented 3 years ago

I am sorry if I hurt your feelings - please be assured that I did not intend any disrespect to you or any of the contributors to this repository.

All we did was moving four lines of code downward by four and I am rather uninterested in who's getting credit or how the commit message adheres to whose standards as long as there's a fix upstreamed to the next release whenever that may be.

Everybody please just relax :)

ederag commented 3 years ago

"Everybody please" don't tell me to "relax" when I just call for mere respect :slightly_smiling_face:. You bypassed my explicit wishes, just after I fixed your issue !

Anyway, if those were your best apologies, then they are accepted. Let's forget this one.