Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utils.py: fix yaml output #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AnakinSolo1983
Copy link

Fixes bug:
#70
with incorrect output for 'Patches' property in .yaml format.

Fixes bug:
getpatchwork#70
with incorrect output for 'Patches' property in .yaml format.

Signed-off-by: Oleg Ananiev <[email protected]>
@rossburton
Copy link

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.

@AnakinSolo1983
Copy link
Author

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?

@stephenfin
Copy link
Member

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.

@AnakinSolo1983
Copy link
Author

Pardon, Stephen. I am a bit confused on what you answered.

@AnakinSolo1983
Copy link
Author

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.

@stephenfin
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants