git-pw
git-pw copied to clipboard
utils.py: fix yaml output
Fixes bug: https://github.com/getpatchwork/git-pw/issues/70 with incorrect output for 'Patches' property in .yaml format.
I'm still not the maintainer but "fix yaml output" doesn't really explain what or why the changes are happening. Please explain in the commit log what the intention of the change is.
Sure, I am okay with writing a more detailed explanation. The question is where will be the best place to do it?
Should it be written as comments inside the code or written as a commit message or ...? Right now I don't see any comments in either the code itself or the commit logs.
Stephen, what will be your preferences as a maintainer?
Comments in the code, please. I suspect this might not be the right approach, however. You'll see we're transforming the response from the API before we call the output functions (e.g. echo_via_pager). At that point, we've already lost a lot of data. We probably want to pass the raw data to the output functions and let that determine how to transform things (e.g. converting 2023-01-23 10:23 to a day ago or not). To do that, we'll also need to provide a hint so the output functions know the "type" of data. How about something like this, defined in e.g. git_pw/_fields.py?
import abc
import arrow
class Field(abc.ABC):
def __init__(self, value):
self._value = value
@abc.abstractmethod
def human_readable(self):
...
@abc.abstractmethod
def machine_readable(self):
...
class DateField(Field):
def human_readable(self):
return arrow.get(self._value).humanize()
def machine_readable(self):
return self._value
...
From a quick look, we'll want a field type for boolean values, submitters, users, and projects. There may be more.
I'm open to other suggestions also, but I do think we want to avoid transforming data before we attempt to output it.
Pardon, Stephen. I am a bit confused on what you answered.
Hello, Stephen. On January this year, I have made a commit, fixing the bug in for the output Patches' property in .yaml format. Since then, you have not told me what will be your preferences as a maintainer, and I am a bit confused on what you commented on January 25th.
Can you please tell me what exactly are your preferences and tell me if there are any issues with the commit that I have submitted?
Thank you.
I don't think what we've done here is sufficient. What I was trying to say is that the data we pass to the echo_via_pager/echo functions (via the output argument) has already been serialized and we don't want serialized data for YAML output (and probably JSON output) since that supports richer data types. For example, here's the tabular output with one entry:
+---------+--------------+--------------------------------------------------------------------------+--------------------------------------+---------+------------+------------+
| ID | Date | Name | Submitter | State | Archived | Delegate |
|---------+--------------+--------------------------------------------------------------------------+--------------------------------------+---------+------------+------------|
| 1911548 | 4 months ago | [9/9] release-notes: Add release notes for the patch note feature | andrepapoti (***********@gmail.com) | new | no | |
|---------+--------------+--------------------------------------------------------------------------+--------------------------------------+---------+------------+------------|
This is the current output for -f yaml:
- archived: 'no'
date: 4 months ago
delegate: ''
id: 1911548
name: '[9/9] release-notes: Add release notes for the patch note feature'
state: new
submitter: *********** (***********@gmail.com)
But we probably want to expose a richer format closer to what we get from the API. Something like this:
- archived: false
date: 2024-03-13T06:56:00Z
delegate: null
id: 1911548
name: '[9/9] release-notes: Add release notes for the patch note feature'
state: new
submitter:
name: ***********
email: ***********@gmail.com
I'm open to idea on how we can do this. My suggestion above was to wrap the unserialized data in formatter classes (so a datatime-style field would get wrapped in a datatime formatter) and pass this wrapped, unserialized data to echo_via_pager and echo. That's just an idea though.
Hopefully that's a little clearer. I don't know if this is something you'd be comfortable doing. I did have a solution started on this way back but I have yet to finish it, unfortunately.