#671: convert str to Fact before check

[aquaherd]

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 .

[matthijskooijman]

@ederag Thanks for looking into this, @aquaherd thanks for putting this into a PR. Looks like a good fix for the update_fact method in isolation. I haven't tested the code, but it looks like both of you did, so I'm approving this.

As a followup to this (or maybe do it directly, since it would also fix this problem), I wonder if it would be good to handle the # to be removed once update facts use Fact directly. comment by moving the string-to-fact conversion (i.e. the Fact.parse() and fact.copy lines) into Storage.UpdateFact(), since I believe that is the only call to update_fact that currently (and always) passes a string. This would also allow removing the start_time and end_time arguments, which are then completely unused. @ederag, would you agree, or is there some indirect call still possible that needs to keep compatibility?

[ederag]

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

[matthijskooijman]

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]

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]

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]

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

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]

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]

would make it less clear who has finalized this commit.

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

[ederag]

@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]

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]

"Everybody please" don't tell me to "relax" when I just call for mere respect 🙂.
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.